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

Sea Turtles - Adriana Gutierrez, Esther Annorzie, Joanna Dudley, & Lili Parra #25

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

Conversation

jfdudley
Copy link

No description provided.

jfdudley and others added 30 commits June 27, 2022 14:44
Copy link

@kelsey-steven-ada kelsey-steven-ada left a comment

Choose a reason for hiding this comment

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

Nice looking API folks! Let me know if you have any questions on the feedback =]

Comment on lines +49 to +54
if data_dict.keys() == cls.required_attributes.keys():
return cls(title=data_dict["title"], owner=data_dict["owner"])
else:
remaining_keys= set(data_dict.keys())-set("title", "owner")
response=list(remaining_keys)
raise ValueError(response)

Choose a reason for hiding this comment

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

Depending on your project, if a user sends an extra key along with the required data for an operation, you may want to still create an object with that valid info and maybe share a message that non-required keys were not be used. There's no right or wrong in this case, just something to think about when designing endpoints.





Choose a reason for hiding this comment

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

Love all the helper functions!

Comment on lines +28 to +36
def update_self(self, data_dict):
dict_key_errors = []
for key in data_dict.keys():
if hasattr(self, key):
setattr(self, key, data_dict[key])
else:
dict_key_errors.append(key)
if dict_key_errors:
raise ValueError(dict_key_errors)

Choose a reason for hiding this comment

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

I think we could cut down on duplication of the identical update_self instance functions each of the classes implements. Looking at where self is used, I believe this could be moved to the helper functions file shared amongst the classes if the first parameter was the instance variable to be updated rather than self.

error_message("Message not found", 400)

request_body["board_id"] = board_id
card = Card.create_from_dict(request_body)

Choose a reason for hiding this comment

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

Could we also use the helper create_record_safely here?

assert response.status_code == 200
assert response_body == []

def test_get_one_board_with_cards(client, one_board_no_cards):

Choose a reason for hiding this comment

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

I think it's important to be really clear about what we are testing with our naming. This tests takes in a board with no cards, adds 2 cards, then fetches it to check the contents. To me, the adding cards portion is important to note, since we are executing additional code than if we had a test which passed in a fixture of a board that already had 2 cards attached to it and immediately fetched that board.

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.

5 participants