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

Making error responses more verbose. #73

Merged
merged 3 commits into from
Jul 19, 2018
Merged

Conversation

guyandtheworld
Copy link
Member

No description provided.

@guyandtheworld guyandtheworld force-pushed the error-response-fix branch 2 times, most recently from 78e4f46 to 916b682 Compare July 6, 2018 10:26
@@ -15,7 +15,7 @@ def main(ctx):
"""
Copy link
Member

Choose a reason for hiding this comment

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

@isht3 I think we should also add the details about the CHALLENGE & PHASE keywords, so that the end user knows that we are referring to the challenge id and phase id here.

@guyandtheworld
Copy link
Member Author

@RishabhJain2018 Yes, agreed.

@guyandtheworld
Copy link
Member Author

@RishabhJain2018 Changed!

@@ -224,7 +224,8 @@ def display_challenge_phase_list(challenge_id):
except requests.exceptions.HTTPError as err:
if (response.status_code in EVALAI_ERROR_CODES):
validate_token(response.json())
echo(style("Error: {}".format(response.json()["error"]), fg="red", bold=True))
echo(style("\nError: {}".format(response.json()["error"]), fg="red", bold=True))
Copy link
Contributor

Choose a reason for hiding this comment

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

A better way to get the error message would be response.json().get("error", "{0} error".format(response.status_code)).

Copy link
Member Author

Choose a reason for hiding this comment

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

@varunagrawal It is repeated throughout the code, wouldn't it be better to solve it as a different issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah guess that makes sense.

echo(style("Error: {}".format(response.json()["error"]), fg="red", bold=True))
echo(style("\nError: {}".format(response.json()["error"]), fg="red", bold=True))
echo(style("\nUse `evalai challenges` to fetch the active challenges.", fg="red", bold=True))
echo(style("\nUse `evalai challenge CHALLENGE phases` to fetch the active phases.\n", fg="red", bold=True))
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason this line repeated so many times? Is there no better way to do this?

Asking so I can understand your thought process and recommend approaches. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Now that I think about it,

echo(style("\nError: {}"
"Use `evalai challenges` to fetch the active challenges."
"Use `evalai challenge CHALLENGE phases` to fetch the active"
"phases.\n".format(response.json()["error"]),
fg="red", bold=True))

Looks much better.

What do you think @varunagrawal @RishabhJain2018 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, this looks nice since it captures all the instructions together.

@guyandtheworld guyandtheworld force-pushed the error-response-fix branch 2 times, most recently from 833ef8d to 14cb410 Compare July 11, 2018 06:14
@guyandtheworld
Copy link
Member Author

@RishabhJain2018 Conflicts fixed!

Copy link
Member

@RishabhJain2018 RishabhJain2018 left a comment

Choose a reason for hiding this comment

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

LGTM :)

@RishabhJain2018
Copy link
Member

@isht3 please resolve conflicts here!

@guyandtheworld
Copy link
Member Author

@RishabhJain2018 Done!

@RishabhJain2018 RishabhJain2018 merged commit bd1936e into master Jul 19, 2018
@RishabhJain2018 RishabhJain2018 deleted the error-response-fix branch July 19, 2018 13:09
Ram81 pushed a commit to Ram81/evalai-cli that referenced this pull request Nov 9, 2020
* Fix error responses in challenges

* Fix error responses in teams

* Fix conflicts
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.

3 participants