Visualization - Error Checking #1610
Conversation
ABrain7710
commented
Feb 3, 2022
•
ABrain7710
commented
- Add method to get the repo_id, start_date, and end_date in the pr reports (this code was repeated for every single vis, so I made it into a method)
- Add a message to the user when they hit an endpoint, but do not specify a repo_id to show data for
- Change name of two methods in contributor reports, because they did not match the endpoint path
Signed-off-by: Andrew Brain <61482022+ABrain7710@users.noreply.github.com>
Signed-off-by: Andrew Brain <61482022+ABrain7710@users.noreply.github.com>
Signed-off-by: Andrew Brain <61482022+ABrain7710@users.noreply.github.com>
…date Signed-off-by: Andrew Brain <61482022+ABrain7710@users.noreply.github.com>
Signed-off-by: Andrew Brain <61482022+ABrain7710@users.noreply.github.com>
Signed-off-by: Andrew Brain <61482022+ABrain7710@users.noreply.github.com>
Signed-off-by: Andrew Brain <61482022+ABrain7710@users.noreply.github.com>
There was a problem hiding this comment.
Not necessarily asking for changes. Except for one comment regarding whether or not we are changing the APIs to plural, everything else is to make sure I understand. I stopped commenting where they became repetitive in different endpoings.
I do think we'll need to provide legacy API endpoints if the pluralization is changing them because of the augur_view interface, as well as a series of Jupyter Notebooks being developed in the CHAOSS Metrics Models working group. (Every other tuesday at 6pm. Next meeting is February 15.) ... essentially, we have a "contract" with other developers that these endpoints will exist as they are. We don't even have to (and probably don't want to) document the old endpoints. We just want them to be "aliased" to the new ones, if that's a thing. Certainly better than redundant code, so I am hoping its a thing. "Feels" like its gotta be ...
| @@ -356,11 +356,14 @@ def get_repo_id_start_date_and_end_date(): | |||
|
|
|||
| now = datetime.datetime.now() | |||
|
|
|||
| repo_id = int(request.args.get('repo_id')) | |||
There was a problem hiding this comment.
Any reason to remove the int cast? I have noticed sometimes python does not do type conversion the way we expect, though that's from APIs we pull from, not our own APIs. Just asking.
There was a problem hiding this comment.
If 'repo_id' is in the request.args array is None (which means it isn't specified in the query parameters), casting it to an int would result in an error because you cannot cast None to an int. Therefore I moved the cast to after I've checked if the 'repo_id' is specified in the request.args array
| return repo_id, start_date, end_date | ||
| if repo_id: | ||
| return int(repo_id), start_date, end_date | ||
|
|
There was a problem hiding this comment.
is the none, none, none API return set what generates the "Hey, you need a repo ID there pardner?" message.
There was a problem hiding this comment.
Yes, basically if the repo_id is not in the query params, then it returns None for all return variables. I could have just returned None, but I decided to still return None for the start_date and end_date too.
| @@ -572,6 +575,12 @@ def new_contributors_bar(): | |||
|
|
|||
| repo_id, start_date, end_date = get_repo_id_start_date_and_end_date() | |||
|
|
|||
| if repo_id is None: | |||
There was a problem hiding this comment.
Seems to be "yes" to my prior question.
| @@ -931,10 +946,16 @@ def new_contributors_stacked_bar(): | |||
|
|
|||
| @server.app.route('/{}/contributor_reports/returning_contributors_pie_chart/'.format(server.api_version), | |||
| methods=["GET"]) | |||
| def returning_contributor_pie_chart(): | |||
There was a problem hiding this comment.
is this simply a convention change, or is It fixing an error. LGTM, I just want to make sure I understand the change.
Does it change the name of the existing endpoint? If so, @Ulincsys is relying on the current names and we probably need to also provide a legacy alias.
There was a problem hiding this comment.
This is simply a change to the method name to match the API endpoint path. It will not change the existing endpoint.
| @@ -1058,10 +1079,16 @@ def returning_contributor_pie_chart(): | |||
|
|
|||
| @server.app.route('/{}/contributor_reports/returning_contributors_stacked_bar/'.format(server.api_version), | |||
| methods=["GET"]) | |||
| def returning_contributor_stacked_bar(): | |||
| def returning_contributors_stacked_bar(): | |||
There was a problem hiding this comment.
Yep, it is just a change so the method name matches the current API endpoint path
|
|
||
| now = datetime.datetime.now() | ||
|
|
||
| repo_id = int(request.args.get('repo_id')) |
There was a problem hiding this comment.
Same question about int casting. I'm just asking to make sure I understand. My guess is you have tested it, and it is not necessary. So, then no problem.
There was a problem hiding this comment.
Yes, it is casted in the return statement, after I've confirmed that it isn't a None type, because trying to cast a None type to int will return an error, and give the user a 500 server error
| @@ -567,11 +590,14 @@ def average_commits_per_PR(): | |||
| @server.app.route('/{}/pull_request_reports/average_comments_per_PR/'.format(server.api_version), methods=["GET"]) | |||
| def average_comments_per_PR(): | |||
|
|
|||
| now = datetime.datetime.now() | |||
There was a problem hiding this comment.
What happens if somebody provides a start date, but not an end date? Is it defaulted to "now" elsewhere?
There was a problem hiding this comment.
If someone provides a start date but no end date, the end date defaults to now. This made me think that I should add an error check to make sure the start_date is before the end_date though.
