resolve error messages #6374
resolve error messages #6374
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Eh2406 (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
This is technically excellent, and I think it is an improvement. Now to the hard part of programing, the people. #5452 had several good, and contradictory, advice for how to improve the wording. Specifically that the |
|
☔ The latest upstream changes (presumably #6400) made this pull request unmergeable. Please resolve the merge conflicts. |
| let req = req.first().unwrap(); | ||
| write!( | ||
| dep_path_desc, | ||
| "\n ... selected to fulfill the requirement \ |
There was a problem hiding this comment.
| "\n ... selected to fulfill the requirement \ | |
| "\n ... selected to satisfy the requirement \ |
"Satisfying" dependencies is already a term that a lot of people are familiar with,
even (or especially) when English isn't their first language.
Using this would probably make it more clear.
(just randomly found this PR, sorry for sort of necromancing, in case this has been resolved elsewhere)
|
Ping @Dylan-DPC or @Eh2406 are you interested in moving this towards completion? It looks like generally this improves things. It also looks like there are some interesting suggestions in #5452 on how to make it clearer. I also like spacekookie's suggestion. |
|
There has been a lot of good thinking about how to make this better, I am still available to help anyone that volunteers to make it happen. |
|
@Eh2406 I'd generally be interested to get my hands dirty in cargo some more :) (assuming @Dylan-DPC doesn't have time to finish it) |
|
I think it is just finding a wording that is easier to read and less stilted. |
|
@spacekookie yeah you can go ahead and finish it no issues. Rewording the error message is the only thing left on this (besides resolving the conflict with latest changes) Thanks |
|
Looks like this was reopened as #6665, so closing in favor of that. |
Improve resolver message to include dependency requirements Resolves #6199. Thanks for previous efforts: #5452, #6374, #6665, which are great but somehow outdated, so I tweak them and create this PR. This will also be obsolete if we ship pubgrub-rs with cargo in the future 😃 But before that happens, IMO these changes are still helpful. --- This PR changes the resolver error message from https://github.com/rust-lang/cargo/blob/216f915c46b8ada2323423d049314ba18247ef95/tests/testsuite/build.rs#L1104-L1106 to https://github.com/rust-lang/cargo/blob/0afd40b4de17a5c45145a0762beb4ef001720fe1/tests/testsuite/build.rs#L1104-L1106 Also provide different message for different source kinds, such like: https://github.com/rust-lang/cargo/blob/0afd40b4de17a5c45145a0762beb4ef001720fe1/tests/testsuite/build.rs#L2810-L2812 ## TODO? From #5452 (comment), there shall be at least one task left behind: > 3. Special case pind by a lock file and not a `"=1.1.2"` in a dependency. Also add a "note: try cargo update" to the end. In this PR, `validate_links` also faces this issue that a dependency requirement is locked into a precise version `=0.1.0`. https://github.com/rust-lang/cargo/blob/a5f8bc94f5d38539dd127f735ea4d3a515c230fd/tests/testsuite/build_script.rs#L1002-L1004 I am uncertain about how to resolve this. Besides the function`validate_links`, is this problem really a thing that may happen? If not, since `validate_links` only handles old validation logic, it may be ok to drop the commit a5f8bc9 and leave it as is.
Closes #6199
r? @Eh2406