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.

Sign up for GitHub

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

Visualization - Error Checking #1610

Merged
merged 7 commits into from
Feb 7, 2022
Merged

Visualization - Error Checking #1610

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
28a6ec6
Detect when repo_id not specified
ABrain7710 Feb 2, 2022
ce5a0d4
Add error telling user if the repo_id is not specified
ABrain7710 Feb 2, 2022
4e8f4d3
Move duplicate code to method, and notify user if repo_id not specified
ABrain7710 Feb 2, 2022
166d84d
Add doc string for method that gets the repo_id, start_date, and end_…
ABrain7710 Feb 2, 2022
fd8b15e
Change method names of methods that didn't match endpoint path
ABrain7710 Feb 2, 2022
8fd0839
Change invalid query param error, to a 400 error code
ABrain7710 Feb 2, 2022
04c0cf5
Add no repo_id error message
ABrain7710 Feb 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 31 additions & 4 deletions augur/routes/contributor_reports.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'))
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

repo_id = request.args.get('repo_id')
start_date = str(request.args.get('start_date', "{}-01-01".format(now.year - 1)))
end_date = str(request.args.get('end_date', "{}-{}-{}".format(now.year, now.month, now.day)))

return repo_id, start_date, end_date
if repo_id:
return int(repo_id), start_date, end_date

Copy link
Member

Choose a reason for hiding this comment

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

is the none, none, none API return set what generates the "Hey, you need a repo ID there pardner?" message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

return None, None, None

def filter_out_repeats_without_required_contributions_in_required_time(repeat_list, repeats_df, required_time,
first_list):
Expand Down Expand Up @@ -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:
Copy link
Member

Choose a reason for hiding this comment

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

Seems to be "yes" to my prior question.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

return Response(response="repo_id not specified. Use this endpoint to get a list of available repos: "
"http://<your_host>/api/unstable/repos",
mimetype='application/json',
status=400)

group_by, required_contributions, required_time = get_new_cntrb_bar_chart_query_params()

input_df = new_contributor_data_collection(repo_id=repo_id, required_contributions=required_contributions)
Expand Down Expand Up @@ -737,6 +746,12 @@ def new_contributors_stacked_bar():

repo_id, start_date, end_date = get_repo_id_start_date_and_end_date()

if repo_id is None:
return Response(response="repo_id not specified. Use this endpoint to get a list of available repos: "
"http://<your_host>/api/unstable/repos",
mimetype='application/json',
status=400)

group_by, required_contributions, required_time = get_new_cntrb_bar_chart_query_params()

input_df = new_contributor_data_collection(repo_id=repo_id, required_contributions=required_contributions)
Expand Down Expand Up @@ -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():
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is simply a change to the method name to match the API endpoint path. It will not change the existing endpoint.

def returning_contributors_pie_chart():

repo_id, start_date, end_date = get_repo_id_start_date_and_end_date()

if repo_id is None:
return Response(response="repo_id not specified. Use this endpoint to get a list of available repos: "
"http://<your_host>/api/unstable/repos",
mimetype='application/json',
status=400)

required_contributions = int(request.args.get('required_contributions', 4))
required_time = int(request.args.get('required_time', 365))

Expand Down Expand Up @@ -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():
Copy link
Member

Choose a reason for hiding this comment

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

Same note as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, it is just a change so the method name matches the current API endpoint path


repo_id, start_date, end_date = get_repo_id_start_date_and_end_date()

if repo_id is None:
return Response(response="repo_id not specified. Use this endpoint to get a list of available repos: "
"http://<your_host>/api/unstable/repos",
mimetype='application/json',
status=400)

group_by = str(request.args.get('group_by', "quarter"))
required_contributions = int(request.args.get('required_contributions', 4))
required_time = int(request.args.get('required_time', 365))
Expand Down
106 changes: 75 additions & 31 deletions augur/routes/pull_request_reports.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,14 +421,37 @@ def filter_data(df, needed_columns, not_null_columns=[]):
print("Developer error, not null columns should be a subset of needed columns")
return df

@server.app.route('/{}/pull_request_reports/average_commits_per_PR/'.format(server.api_version), methods=["GET"])
def average_commits_per_PR():
def get_repo_id_start_date_and_end_date():

""" Gets the repo_id, start_date, and end_date from the GET requests array

:return: repo_id - id of the repo data is being retrieved for
:return: start_date - earliest time on visualization. Defaults to the January 1st of last year
:return: end_date - latest time on visualization. Defaults to current date
"""

now = datetime.datetime.now()

repo_id = int(request.args.get('repo_id'))
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

repo_id = request.args.get('repo_id')
start_date = str(request.args.get('start_date', "{}-01-01".format(now.year - 1)))
end_date = str(request.args.get('end_date', "{}-{}-{}".format(now.year, now.month, now.day)))

if repo_id:
return int(repo_id), start_date, end_date

return None, None, None

@server.app.route('/{}/pull_request_reports/average_commits_per_PR/'.format(server.api_version), methods=["GET"])
def average_commits_per_PR():

repo_id, start_date, end_date = get_repo_id_start_date_and_end_date()

if repo_id is None:
return Response(response="repo_id not specified. Use this endpoint to get a list of available repos: "
"http://<your_host>/api/unstable/repos",
mimetype='application/json',
status=400)

group_by = str(request.args.get('group_by', "month"))
return_json = request.args.get('return_json', "false")

Expand Down Expand Up @@ -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()
Copy link
Member

Choose a reason for hiding this comment

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

What happens if somebody provides a start date, but not an end date? Is it defaulted to "now" elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

repo_id, start_date, end_date = get_repo_id_start_date_and_end_date()

if repo_id is None:
return Response(response="repo_id not specified. Use this endpoint to get a list of available repos: "
"http://<your_host>/api/unstable/repos",
mimetype='application/json',
status=400)

repo_id = int(request.args.get('repo_id'))
start_date = str(request.args.get('start_date', "{}-01-01".format(now.year - 1)))
end_date = str(request.args.get('end_date', "{}-{}-{}".format(now.year, now.month, now.day)))
return_json = request.args.get('return_json', "false")

df_type = get_df_tuple_locations()
Expand Down Expand Up @@ -747,11 +773,14 @@ def average_comments_per_PR():
methods=["GET"])
def PR_counts_by_merged_status():

now = datetime.datetime.now()
repo_id, start_date, end_date = get_repo_id_start_date_and_end_date()

if repo_id is None:
return Response(response="repo_id not specified. Use this endpoint to get a list of available repos: "
"http://<your_host>/api/unstable/repos",
mimetype='application/json',
status=400)

repo_id = int(request.args.get('repo_id'))
start_date = str(request.args.get('start_date', "{}-01-01".format(now.year - 1)))
end_date = str(request.args.get('end_date', "{}-{}-{}".format(now.year, now.month, now.day)))
return_json = request.args.get('return_json', "false")

x_axis = 'closed_year'
Expand Down Expand Up @@ -937,11 +966,14 @@ def PR_counts_by_merged_status():
methods=["GET"])
def mean_response_times_for_PR():

now = datetime.datetime.now()
repo_id, start_date, end_date = get_repo_id_start_date_and_end_date()

if repo_id is None:
return Response(response="repo_id not specified. Use this endpoint to get a list of available repos: "
"http://<your_host>/api/unstable/repos",
mimetype='application/json',
status=400)

repo_id = int(request.args.get('repo_id'))
start_date = str(request.args.get('start_date', "{}-01-01".format(now.year - 1)))
end_date = str(request.args.get('end_date', "{}-{}-{}".format(now.year, now.month, now.day)))
return_json = request.args.get('return_json', "false")

df_type = get_df_tuple_locations()
Expand Down Expand Up @@ -1234,11 +1266,14 @@ def add_legend(location, orientation, side):
methods=["GET"])
def mean_days_between_PR_comments():

now = datetime.datetime.now()
repo_id, start_date, end_date = get_repo_id_start_date_and_end_date()

if repo_id is None:
return Response(response="repo_id not specified. Use this endpoint to get a list of available repos: "
"http://<your_host>/api/unstable/repos",
mimetype='application/json',
status=400)

repo_id = int(request.args.get('repo_id'))
start_date = str(request.args.get('start_date', "{}-01-01".format(now.year - 1)))
end_date = str(request.args.get('end_date', "{}-{}-{}".format(now.year, now.month, now.day)))
return_json = request.args.get('return_json', "false")

time_unit = 'Days'
Expand Down Expand Up @@ -1399,11 +1434,14 @@ def mean_days_between_PR_comments():
@server.app.route('/{}/pull_request_reports/PR_time_to_first_response/'.format(server.api_version), methods=["GET"])
def PR_time_to_first_response():

now = datetime.datetime.now()
repo_id, start_date, end_date = get_repo_id_start_date_and_end_date()

if repo_id is None:
return Response(response="repo_id not specified. Use this endpoint to get a list of available repos: "
"http://<your_host>/api/unstable/repos",
mimetype='application/json',
status=400)

repo_id = int(request.args.get('repo_id'))
start_date = str(request.args.get('start_date', "{}-01-01".format(now.year - 1)))
end_date = str(request.args.get('end_date', "{}-{}-{}".format(now.year, now.month, now.day)))
return_json = request.args.get('return_json', "false")
remove_outliers = str(request.args.get('remove_outliers', "true"))

Expand Down Expand Up @@ -1533,11 +1571,14 @@ def PR_time_to_first_response():
methods=["GET"])
def average_PR_events_for_closed_PRs():

now = datetime.datetime.now()
repo_id, start_date, end_date = get_repo_id_start_date_and_end_date()

if repo_id is None:
return Response(response="repo_id not specified. Use this endpoint to get a list of available repos: "
"http://<your_host>/api/unstable/repos",
mimetype='application/json',
status=400)

repo_id = int(request.args.get('repo_id'))
start_date = str(request.args.get('start_date', "{}-01-01".format(now.year - 1)))
end_date = str(request.args.get('end_date', "{}-{}-{}".format(now.year, now.month, now.day)))
return_json = request.args.get('return_json', "false")
include_comments = str(request.args.get('include_comments', True))

Expand Down Expand Up @@ -1719,11 +1760,14 @@ def average_PR_events_for_closed_PRs():
@server.app.route('/{}/pull_request_reports/Average_PR_duration/'.format(server.api_version), methods=["GET"])
def Average_PR_duration():

now = datetime.datetime.now()
repo_id, start_date, end_date = get_repo_id_start_date_and_end_date()

repo_id = int(request.args.get('repo_id'))
start_date = str(request.args.get('start_date', "{}-01-01".format(now.year - 1)))
end_date = str(request.args.get('end_date', "{}-{}-{}".format(now.year, now.month, now.day)))
if repo_id is None:
return Response(response="repo_id not specified. Use this endpoint to get a list of available repos: "
"http://<your_host>/api/unstable/repos",
mimetype='application/json',
status=400)

group_by = str(request.args.get('group_by', "month"))
return_json = request.args.get('return_json', "false")
remove_outliers = str(request.args.get('remove_outliers', "true"))
Expand Down