Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tien&Anya - Sharks Solar_system #27

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

Tien&Anya - Sharks Solar_system #27

wants to merge 19 commits into from

Conversation

AnnnAPr
Copy link

@AnnnAPr AnnnAPr commented May 2, 2022

No description provided.

Copy link

@spitsfire spitsfire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part 2! Great job, Tien and Anya! I didn't see any red flags or things I was concerned about!

Comment on lines +14 to +15
# app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False
# app.config['SQLALCHEMY_DATABASE_URI'] = 'postgresql+psycopg2://postgres:postgres@localhost:5432/solar_system_development'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we now have this string stored in our environmental variables for safe keeping we want to get rid of it from public view

Suggested change
# app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False
# app.config['SQLALCHEMY_DATABASE_URI'] = 'postgresql+psycopg2://postgres:postgres@localhost:5432/solar_system_development'

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, should be deleted from init.py

@@ -0,0 +1,15 @@
from flask import abort, make_response

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Great job moving this helper function into a separate file, so that our routes are more clean and easy to read!

Comment on lines +34 to +47
# class Planet():
# def __init__(self, id, name, description, moons = None):
# self.id = id
# self.name = name
# self.description = description
# self.moons = moons

# def to_json(self):
# return {
# "id": self.id,
# "name" : self.name,
# "decription": self.description,
# "moons": self.moons
# }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# class Planet():
# def __init__(self, id, name, description, moons = None):
# self.id = id
# self.name = name
# self.description = description
# self.moons = moons
# def to_json(self):
# return {
# "id": self.id,
# "name" : self.name,
# "decription": self.description,
# "moons": self.moons
# }

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot delete those lines

"moons": self.moons
}

def update(self, req_body):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 helper method woo

db.session.add(new_planet)
db.session.commit()

# return make_response(jsonify(f"Planet {new_planet.name} successfully created!", 201))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks the same as the one below, so let's get rid of this

Suggested change
# return make_response(jsonify(f"Planet {new_planet.name} successfully created!", 201))

Comment on lines +41 to +42
# return jsonify(planet.to_json()), 200
return jsonify(planet.to_json())

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason you removed the status code on line 42?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think no. I am not sure why we removed it.

@@ -0,0 +1,42 @@
import pytest

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 looks good

@@ -0,0 +1,45 @@
def test_get_all_planets_with_no_records(client):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 looks good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants