Skip to content

Add methods for path lookups in test_install_{extras, index}.py #8326

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

Merged
merged 1 commit into from
Jun 9, 2020

Conversation

ssurbhi560
Copy link
Contributor

towards #6050

Copy link
Contributor

@gutsytechster gutsytechster left a comment

Choose a reason for hiding this comment

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

You've not provided the message arguments at some places. That's the only thing I guess, which is left, else LGTM :) Please correct if I get some reviews wrong.
Please find the reviews inline.

@ssurbhi560
Copy link
Contributor Author

@deveshks @gutsytechster , Thank you for the review, will make these changes!

@pradyunsg
Copy link
Member

pradyunsg commented May 27, 2020

I don't think any of the suggested changes are appropriate. All of them are suggesting setting message to something that's already included in str(result) in a much more readable form.

message should be a human readable string describing the error. To reinforce that idea, I strongly suggest only passing it as a keyword argument (the only reason I didn't make it one in the original PR was Python 2 support 😞).

message=result.files_created doesn't read right, does it? :P

Instances like these, where not-all-the-relevant-info messages were provided through the assertion are spread across the test suite (str(result) includes all the context, result.files_created doesn't), and fixing them to provide all the relevant information is the point of this exercise (in addition to the fact that we'd have 1 spot to normalize how paths vs strings are handled in such comparisions).

@ssurbhi560
Copy link
Contributor Author

I was thinking somewhat the same, but wasn't confident.
Thank you @pradyunsg for reviewing, will update the PR. :)

@gutsytechster
Copy link
Contributor

Eh! Thanks for correcting. I'll update my PR as well. :P

@ssurbhi560 ssurbhi560 force-pushed the use_methods_in_tests branch from f5a9323 to 57a0815 Compare May 28, 2020 07:16
@sbidoul sbidoul merged commit 5e69b8c into pypa:master Jun 9, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants