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

Lintcheck: Add URL to lint emission place in diff #13104

Merged
merged 2 commits into from
Jul 18, 2024

Conversation

xFrednet
Copy link
Member

@xFrednet xFrednet commented Jul 15, 2024

This PR adds links to the emission code in our lintcheck CI. When reviewing changes, I would like to be able to see the bigger context, which isn't always included in the lint message. This PR adds a nice link to the lintcheck diff that allows for simple investigation of the code in question.

At first, I wanted to simply use the doc.rs links and call it a day, but then I figured out that some crates might have their source files remapped. Cargo was the crate that made me notice this. As a response, I made the link configurable. (See rust-lang/docs.rs#2551 for a detailed explanation and possible solution to remove this workaround in the future.)

It's probably easiest to review the individual commits. The last one is just a dummy to showcase the change.

🖼️ rendered 🖼️


r? @Alexendoo

changelog: none


That's it, I hope that everyone who's reading this has a beautiful day :D

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 15, 2024
@xFrednet xFrednet force-pushed the 00000-lintcheck-better-md-links branch from a0ba585 to a941454 Compare July 15, 2024 19:55
@xFrednet xFrednet marked this pull request as draft July 15, 2024 20:02
@xFrednet
Copy link
Member Author

It looks like the cargo link is sadly still incorrect 🤔

@xFrednet xFrednet force-pushed the 00000-lintcheck-better-md-links branch 2 times, most recently from 39281e4 to 0592dc1 Compare July 15, 2024 20:07
@xFrednet xFrednet marked this pull request as ready for review July 15, 2024 20:14
@xFrednet
Copy link
Member Author

The cargo links work now 🎉

Some links are currently not 100% correct as the versions we use for lintcheck are so old that they used a previous version of docs.rs link structure. I considered fixing this, but decided to hold of on it, until I update our default and CI test set (Which is on my todo list for next week)

@Alexendoo
Copy link
Member

A few times I've forgotten if I'm reading the Added or Removed sections because they look the same, this would add a convenient place to put a plus/minus or similar, what do you think?

@xFrednet xFrednet force-pushed the 00000-lintcheck-better-md-links branch from 0592dc1 to 9847a7b Compare July 16, 2024 16:44
@xFrednet
Copy link
Member Author

xFrednet commented Jul 16, 2024

I like the idea. A simple + or - would be made into a list by GH's Markdown. I now used the title as a prefix, which constructs messages like this:

Added clippy::cool_lint at https://cataas.com/cat
Removed clippy::cool_lint at https://cataas.com/cat
Changed clippy::cool_lint at https://cataas.com/cat

Edit: 🖼️ rendered 🖼️

Comment on lines 12 to 13
file_name: String,
byte_pos: (u32, u32),
Copy link
Member

Choose a reason for hiding this comment

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

Could remove these now since the file_link would cover it well enough

Copy link
Member Author

Choose a reason for hiding this comment

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

The byte position is still used in the key() function on line 19. We could replace it with the link, but I think having it separately would make more sense. While all links currently contain line labels, there is no guarantee that they will for manually configured lints. And links also don't contain the column, which is included in this field.

Copy link
Member

Choose a reason for hiding this comment

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

Since they're displayed under [lint] at [url] sections I think those making up the key makes sense. Removing information from the key has a benefit - it would mean tweaks to a span's start column/end line/end column show as a change rather than

Added `x` at y#5
...
Removed `x` at y#5

I didn't think about links without line numbers, if that happened more things would appear as changes than necessary but reasonably I don't think we'll have such links

Copy link
Member

Choose a reason for hiding this comment

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

Not critical though, either way it's ready for r+ after ditching the test commit

Copy link
Member Author

Choose a reason for hiding this comment

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

Removing information from the key has a benefit - it would mean tweaks to a span's start column/end line/end column show as a change rather than

The tweaked span would still result in a added and removed message, since the lint message displays the span and is also used as part of the span. I think here it's safer to display them separately and leave it to the reviewer to merge them.

Copy link
Member

Choose a reason for hiding this comment

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

The message isn't part of the key

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I mixed it up with the lint name 😅 But I'd still prefer it to be separate. Some lints might also be emitted multiple times per line.

@xFrednet xFrednet force-pushed the 00000-lintcheck-better-md-links branch from 9847a7b to c2d8cab Compare July 18, 2024 13:14
@xFrednet
Copy link
Member Author

I removed the dummy commit and renamed a variable to avoid clippy::similar_names. This should be good to go from my side

lintcheck/src/input.rs Outdated Show resolved Hide resolved
@xFrednet xFrednet force-pushed the 00000-lintcheck-better-md-links branch from c2d8cab to 0e3d197 Compare July 18, 2024 13:28
@Alexendoo
Copy link
Member

🐰

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 18, 2024

📌 Commit 0e3d197 has been approved by Alexendoo

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jul 18, 2024

⌛ Testing commit 0e3d197 with merge b31bce4...

@bors
Copy link
Collaborator

bors commented Jul 18, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Alexendoo
Pushing b31bce4 to master...

@bors bors merged commit b31bce4 into rust-lang:master Jul 18, 2024
8 checks passed
@xFrednet xFrednet deleted the 00000-lintcheck-better-md-links branch July 18, 2024 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants