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

Test message check fixes #8468

Merged
merged 1 commit into from
Jun 19, 2020
Merged

Test message check fixes #8468

merged 1 commit into from
Jun 19, 2020

Conversation

pfmoore
Copy link
Member

@pfmoore pfmoore commented Jun 18, 2020

This fixes a number of tests where the only issue is that the new resolver gives a different message than the old resolver. I've taken the view that changing the test to reflect what the new resolver says is acceptable, at least in the short term.

It's very arguable that the correct approach is actually to make the new resolver give the "correct" message, but it's not clear to me that this would be straightforward to do, and as we have a UX project working on the broarder task of improving pip's messages, it seems reasonable to make a tactical fix for now. If nothing else, the test code will pinpoint where and how the messages differ between the two resolvers, and we can use that as a starting point for any further improvements.

It's not obvious to me to what extent the existing messages are based on implementation details of the old resolver (in which case, it's fair game to change them based on implementation details of the new resolver) or on a conscious decision to display a particular message to the user (in which case we probably should make more effort to review if the new message is an acceptable alternative). But my inclination is to address that sort of question in follow-up PRs, once we have the test suite failures dealt with.

Opinions, anyone? @pradyunsg @uranusjr @brainwane @ei8fdb @nlhkabu

@pfmoore pfmoore added the skip news Does not need a NEWS file entry (eg: trivial changes) label Jun 18, 2020
@pfmoore pfmoore requested a review from pradyunsg June 18, 2020 17:24
Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

Same reasoning—if the message is going to change soon anyway, let’s implement the new messaging. The current message is “good enough.” (I for one had never understood or cared about the difference between “up-to-date” and “already satisfied” before working on this project.)

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

100% on board.

I'm also really curious to know from user testing if folks find the resolver's current output useful, and how we should change it (if at all).

@pfmoore
Copy link
Member Author

pfmoore commented Jun 19, 2020

OK, I'm going to take that as confirmation this can be merged. We can always revisit these tests later.

@pfmoore pfmoore merged commit 3a3d1d5 into pypa:master Jun 19, 2020
@pfmoore pfmoore deleted the nr_test_messages branch June 19, 2020 10:01
@brainwane
Copy link
Contributor

Thanks everyone. Agreed.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
skip news Does not need a NEWS file entry (eg: trivial changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants