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

Add lines labels to /crate/**/source/ #2551

Closed
xFrednet opened this issue Jul 12, 2024 · 8 comments · Fixed by #2560
Closed

Add lines labels to /crate/**/source/ #2551

xFrednet opened this issue Jul 12, 2024 · 8 comments · Fixed by #2560
Assignees
Labels
A-frontend Area: Web frontend E-easy Effort: Should be easy to implement and would make a good first PR

Comments

@xFrednet
Copy link
Member

xFrednet commented Jul 12, 2024

While generating some links, I noticed that source files under https://docs.rs/crate/{crate}/{version}/source/src/{file} don't support line labels. This is opposed to the sources under https://docs.rs/{crate}/{version}/src/{krate}/{file}.html#{line} that do.

Usually, I'd just use the second link with line label support for everything. However, rustdoc can sometimes remap paths, for example if a path is set in Cargo.toml. This means that the second link doesn't always work. At the end of the issue, I'll give a detailed example of this.

For my use case, the best solution would be to add line numbers and line labels to the sources under crate/{crate}/{version}/source/.

I'd be happy to give the implementation a shot, if this is something that would be accepted and I get some hints where to start 🗡️


Specific example:

We run Clippy in our CI and collect lint emissions. These diagnostics have paths as seen by rustc. It would, for example, look like this: ./src/lib.rs:15. I now want to take these file names and link to docs.rs to help us investigate the lint emissions. For most crates, the link is simply constructed like this:

  • https://docs.rs/{krate}/{version}/src/{krate}/{file}.html#{line}

And this works for most crates, expect crates that remap their path in Cargo.toml, like this (Taken from rust-lang/cargo):

[lib]
name = "cargo"
path = "src/cargo/lib.rs"

The lint emission then has the path ./src/cargo/lib.rs but the documentation generated by rustdoc removes the cargo from the path. So using the previous link template generates:

This doesn't work due to the cargo/cargo. The correct link with the remapped path is:

This remapping only happens for rustdoc and not the source files under crate/ so if those would support line labels, it would be a simple way to always use the same link template.

@syphar syphar added E-easy Effort: Should be easy to implement and would make a good first PR A-frontend Area: Web frontend labels Jul 12, 2024
@syphar
Copy link
Member

syphar commented Jul 12, 2024

I would be totally fine with having this feature and definitely see a use-case.

I'm happy to review any PR adding this. the interesting files are:

@xFrednet
Copy link
Member Author

I'll look into this :D

@rustbot claim

@syphar
Copy link
Member

syphar commented Jul 16, 2024

there is a current big PR (#2292) that will change template rendering, not sure if you want to wait with your changes until it lands, or base your work ontop of it.

@xFrednet
Copy link
Member Author

In that case, I'll wait a bit. Thank you for the early notice :D

@syphar
Copy link
Member

syphar commented Jul 16, 2024

#2292 is merged, so feel free to start @xFrednet :)

@xFrednet
Copy link
Member Author

I've made some decent progress. My next step is now to highlight the line numbers. I found the implementation of rustdoc which seems to work well. Can I copy and adapt it, as long as I add a comment mentioning where it comes from? I'm asking regarding the license etc.

@syphar
Copy link
Member

syphar commented Jul 18, 2024

IANAL, but IMO you can just copy & adapt the code, even without attribution.

@GuillaumeGomez what do you think?

( also about the piece of code )

@GuillaumeGomez
Copy link
Member

Yes it's completely fine. Just need to be careful that the line height of the line number is aligned to the code line.

bors added a commit to rust-lang/rust-clippy that referenced this issue Jul 18, 2024
…exendoo

Lintcheck: Add URL to lint emission place in diff

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.

[:framed_picture: rendered :framed_picture:](https://github.com/rust-lang/rust-clippy/actions/runs/9960834924?pr=13104)

---

r? `@Alexendoo`

changelog: none

---

That's it, I hope that everyone who's reading this has a beautiful day :D
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-frontend Area: Web frontend E-easy Effort: Should be easy to implement and would make a good first PR
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants