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

Link to sourcegraph #1947

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Link to sourcegraph #1947

wants to merge 2 commits into from

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Dec 5, 2022

As discussed in https://rust-lang.zulipchat.com/#narrow/stream/356853-t-docs-rs/topic/sourcegraph.20as.20a.20source.20view.20replacement.

Sourcegraph announced they have all crates indexed as of July this year: https://twitter.com/sourcegraph/status/1547991489149423620

Note: this doesn't yet delete the code that fills the files column, nor that generates archives of source files. That can be done in a followup, after this has been deployed for a while.

@github-actions github-actions bot added the S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed label Dec 5, 2022
@Nemo157
Copy link
Member

Nemo157 commented Dec 5, 2022

For #1866 I was proposing we move to reading the README.md from the source archive so that we don't need to store this in the database.

One issue I have with sourcegraph is that it doesn't work without JS, I would really like a future where 99% of docs.rs works without JS enabled, there's very little we do that should require it.

(I'm also unsure how well their privacy policy fits, they claim to provide information to third-party advertising services with an opt-out. Since we just link to them rather than embed them in the page it's probably fine for them to have a much more expansive data collection system, but it still feels wrong to send users to a site that explicitly appears to violate the GDPR.)

@GuillaumeGomez
Copy link
Member

@Nemo157: This feature without JS is very limited and can basically only allow "jumps" (as you can see in this RFC). From what we discussed on zulip, the end goal would be to use an external tool/library (details to be confirmed/discussed) which would be integrated into rustdoc output directly. But I think JS will be mandatory in any case.

@Nemo157
Copy link
Member

Nemo157 commented Dec 5, 2022

The very limited source view is personally all I expect of it (note that I'm only talking about https://docs.rs/crate/.../source/..., not the rustdoc generated source pages). The major usecase is just to be able to have a quick webview of exactly the source in the archive, to avoid having to cargo dl foobar and browse the source locally. If I want to do anything more complex around exploring the source I would download the archive and browse it locally.

EDIT: If we are able to easily integrate a much more powerful browsing system using JS, but that fallsback to something like we have today without it, I'd be all for that too. I just don't want JS required for the base functionality.

@jyn514
Copy link
Member

jyn514 commented Dec 5, 2022

Sourcegraph announced they have all crates indexed as of July this year

Does that include all crates on crates.io? Or only crates that already have a public git repository?

@jsha jsha force-pushed the link-to-sourcegraph branch from 1da9a43 to 78823d5 Compare December 5, 2022 20:25
@jsha
Copy link
Contributor Author

jsha commented Dec 5, 2022

Does that include all crates on crates.io? Or only crates that already have a public git repository?

Yep, all crates on crates.io, and it loads them from crates.io so it shows the actual contents.

One issue I have with sourcegraph is that it doesn't work without JS, I would really like a future where 99% of docs.rs works without JS enabled, there's very little we do that should require it.

I also like to make things work without JS whenever possible. I think it encourages building better web sites, and can often make things faster to load. But I consider it a tradeoff against our other goals. In this case, we get reduced maintenance, reduced storage, and a dramatically increased feature set. I think that's a worthwhile tradeoff for requiring JS.

Here's a concrete example: It's important to view the actual contents of a crate if you want to diff one crate version against another, for instance when upgrading a dependency. It would be pretty expensive for docs.rs (or crates.io) to do that diff on the server side. We don't currently offer that feature. But on Sourcegraph it's available, and very fast: https://sourcegraph.com/crates/ureq/-/compare/v2.4.0...v2.5.0

@syphar syphar added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Dec 15, 2022
@syphar
Copy link
Member

syphar commented Dec 15, 2022

I was thinking longer about this and ended up thinking that I don't know enough about the rust ecosystem to know how our existing source-view is used & if we can / should replace it with a link to sourcegraph.

How about this:

  • we additionally link to sourcegraph in our menu, next to the source-view, clearly stating that we're opening another site. We could also add a link to each file in the source-view
  • add a metric how often one of the links is clicked.

So we can deploy this, and then watch how both the RPM on our source-views & the redirect clicks develop over time.

@jyn514
Copy link
Member

jyn514 commented Dec 15, 2022

🤷 I don't have a strong objection to that, but I think it will be a rare feature on top of a rare feature, people already mostly use the rustdoc source view instead of the docs.rs source view. So I'm not sure it's worth the effort.

@syphar
Copy link
Member

syphar commented Dec 15, 2022

It's definitely getting requests, that's what makes me think:

grafik

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This PR is incomplete or needs to address review comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants