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

Filter project by completion status #139

Merged
merged 5 commits into from
Mar 14, 2021
Merged

Conversation

peterghrong
Copy link
Collaborator

@peterghrong peterghrong commented Feb 25, 2021

Summary

Filtering projects by the [True, False] completion status

Test Plan

Passed all unit tests
Screen Shot 2021-03-13 at 1 19 12 PM

Which issues does this PR resolve/work on?

Working On #131

@peterghrong peterghrong marked this pull request as ready for review March 13, 2021 18:21
@@ -173,7 +173,107 @@ def test_get_specific_type(self):
json_response = json.loads(response3.get_data(as_text=True))
self.assertEqual(len(json_response), 2)

# test get endpoint by completion status
def test_get_projects_by_completion_status(self):
test_list = [True, False]
Copy link
Member

Choose a reason for hiding this comment

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

is there a reason why you decided to make a boolean array?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not really, thought it would be easier to see the tested inputs when it comes to code review.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, this is more of a nitpick & stylistic preference but I think it would be more readable if you just used the Boolean values instead and I think it's more commonly done that way anyways so the code reader knows exactly what to expect

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the suggestion Celine, and also apologies for the confusion. I have updated the boolean values accordingly

@peterghrong peterghrong merged commit 5db2a58 into main Mar 14, 2021
@peterghrong peterghrong deleted the get-project-by-is_complete branch March 14, 2021 23:15
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.

2 participants