Conversation
kelsey-steven-ada
left a comment
kelsey-steven-ada
left a comment
There was a problem hiding this comment.
Great work Paige! I've left some comments & questions, let me know here or on Slack if there's anything I can clarify 😊
| app = Flask(__name__) | ||
|
|
||
| if not test_config: | ||
| app.config['AQLALCHEMY_TRACK_MODIFICATIONS'] = False |
There was a problem hiding this comment.
It looks like there's a typo in the spelling of the dictionary key, so this setting might not be respected when the server runs.
| if not test_config: | ||
| app.config['AQLALCHEMY_TRACK_MODIFICATIONS'] = False | ||
| app.config['SQLALCHEMY_DATABASE_URI'] = os.environ.get("SQLALCHEMY_DATABASE_URI") | ||
| else: | ||
| app.config["TESTING"] = True | ||
| app.config["SQLALCHEMY_TRACK_MODIFICATIONS"] = False | ||
| app.config["SQLALCHEMY_DATABASE_URI"] = os.environ.get( | ||
| "SQLALCHEMY_TEST_DATABASE_URI") |
There was a problem hiding this comment.
Both side of the if/else contain app.config["SQLALCHEMY_TRACK_MODIFICATIONS"] = False, we could D.R.Y. up our code by moving that line above the if/else:
app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False
if not test_config:
app.config['SQLALCHEMY_DATABASE_URI'] = os.environ.get("SQLALCHEMY_DATABASE_URI")
else:
app.config["TESTING"] = True
app.config["SQLALCHEMY_DATABASE_URI"] = os.environ.get("SQLALCHEMY_TEST_DATABASE_URI")| try: | ||
| model_id = int(model_id) | ||
| except: | ||
| abort(make_response({"message":f"planet {model_id} invalid"}, 400)) |
There was a problem hiding this comment.
Our function will always return text that says planet, we could replace that with something like cls.__name__ if we want to read the name off the class we pass as the first parameter.
| planets_response = [] | ||
| for planet in planets: | ||
| planets_response.append(planet.to_dict()) |
There was a problem hiding this comment.
This would be a great place for a list comprehension:
planets_response = [planet.to_dict() for planet in planets]| db.session.add(new_planet) | ||
| db.session.commit() | ||
|
|
||
| return make_response(jsonify(f"Planet {new_planet.name} successfully created"), 201) |
There was a problem hiding this comment.
Some lines across the file are a little long for style best practices, how could we split them up?
| # planets = [ | ||
| # Planets(1, "Saturn", "Gaseous planet with rings"), | ||
| # Planets(2, "Jupiter", "Strom planet"), | ||
| # Planets(3, "Venus", "Where girls come from") | ||
| # ] No newline at end of file |
There was a problem hiding this comment.
We typically want to delete commented out code before opening PRs. We can rely on our commit history if we need to check out how something used to look =]
There was a problem hiding this comment.
I really appreciate the clarity of the code across the routes, really nice use of descriptive names and spacing to make it easy to read! ^_^
| return app.test_client() | ||
|
|
||
| @pytest.fixture | ||
| def get_one_planet(): |
There was a problem hiding this comment.
Since we aren't returning a planet, I would consider updating the function name to reflect that we're storing one planet in the database. This applies to read_all_planets below as well, how could we rename it to reflect that the function stores 2 planets?
| @@ -0,0 +1,74 @@ | |||
|
|
|||
| def test_len_of_empty_list(client): | |||
There was a problem hiding this comment.
It looks like this test is checking that if the database is empty, we return an empty list for the get all endpoint. How could we rename this test to include those details about the route and input?
|
|
||
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
One of the things to look at when we are in our code clean up phase is whitespace across a file. The general guideline is one new line between functions inside of a class, 2 new lines for top level functions in a file - but what's most important is to be consistent across a codebase. A little extra whitespace is often used as a visual separation between groups of related functions, or to make it clear where imports end and implementation code begins. We lose the ability to have a visual separation have meaning if we are not consistent with how they are applied. The PEP 8 style guide has a little more info on their recommendations: https://peps.python.org/pep-0008/#blank-lines
3 participants
No description provided.