-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Normalize use of backticks compiler messages #61901
Normalize use of backticks compiler messages #61901
Conversation
Some changes occurred in diagnostic error codes |
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cramertj (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. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
r? @alexreg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, apart from the two very minor comments. Thanks a lot for this!
I haven't checked it myself yet, but can you make sure that literal values are all quoted too? i.e., strings, numbers, bools, so you have (e.g.) Actually, this can look kind of awkward with strings, e.g., |
Seems fine to me. |
52f9b9e
to
70ee2f4
Compare
☔ The latest upstream changes (presumably #61983) made this pull request unmergeable. Please resolve the merge conflicts. |
70ee2f4
to
30c1362
Compare
30c1362
to
6682487
Compare
Hello @alexreg! I have updated the PR. Unfortunately, I haven't been able to find debug messages with literal values that are not quoted...But maybe my regexes weren't accurate. For this PR, I've squashed the 3 commits into one. Would you prefer 3 seperate commits for easier review? (especially for the last big commit where the tests were update by the |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@fakenine Thanks a lot. Yes, 3 separate commits would be best. I already reviewed the first one (presuming you didn't amend it), so that would save time. As for literal values, probably this will have to be done manually I'm afraid. But the good news is there are probably many fewer examples of this. Maybe identify them from Looks like you forgot to bless the run-pass tests BTW. :-) |
☔ The latest upstream changes (presumably #61778) made this pull request unmergeable. Please resolve the merge conflicts. |
6682487
to
d82c598
Compare
☔ The latest upstream changes (presumably #61682) made this pull request unmergeable. Please resolve the merge conflicts. |
@fakenine Looks like there's just a few more ui tests to bless... if you could do that, fix the little conflict (just rebase and bless the corresponding test), and make the small change I commented on, I'll review and hopefully we can get this merged. :-) |
Hello @alexreg! Thank you for the feedback :) I'll bless the remaining tests and apply the requested changes (blessing takes the most time because my computer takes quite a long time to build hehe). I will notify you when everything is ready for me! |
d82c598
to
6009ab8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall very good.
Great, thanks. Just a few more things to address (pre-existing, so not your fault), and we're good to go. If you have trouble blessing all the tests As for building, yeah, it's a pain... make sure you do |
@alexreg Oops! My bad, that's noted for next time. I'll fix this |
@fakenine I’d already fixed it myself before posting that comment. |
☔ The latest upstream changes (presumably #62419) made this pull request unmergeable. Please resolve the merge conflicts. |
00bfb8d
to
4448b27
Compare
@bors r+ |
📌 Commit 4448b27 has been approved by |
⌛ Testing commit 4448b27 with merge c62d6f0f62b481e7078f8cdc85adfe9087a606e8... |
Tests seem to have failed in the PR builder so @bors r- retry Please consider breaking this PR up into several independent pieces. |
@fakenine Looks like some recent merges created new tests that need blessing... sorry! https://dev.azure.com/rust-lang/rust/_build/results?buildId=2537 |
@bors delegate=fakenine You can now r+ it in a similar manner to how I did above, once you've reblessed the tests. (Make sure you pull my latest version from your branch before you edit it though.) Maybe ping @Centril once you've r+'ed too, in case I'm not around. He can make sure it gets through without further conflicts.. hopefully! |
✌️ @fakenine can now approve this pull request |
Note: please do @ bors r=alexreg and not @ bors r+, since the latter will incorrectly approve this PR in your name, not alexreg's. The spaces here shouldn't be replicated, that's merely because bors is very eager in finding invocations to it. |
Yes sorry, @Mark-Simulacrum is right. It's very late here. -_- |
@Centril I’ll do that. I’m closing this PR and will split it to avoid multiple rebasings and conflicts |
Normalize use of backticks compiler messages
Fixes #60532