Do not attempt to overwrite docs.rs link.

[DoumanAsh]

Subj to fix #1206 issue

P.s. a better solution should be done to perform update on back-end once that would handle all variants of docs.rs links

[kureuil]

@DoumanAsh Thanks for your work on this issue, I'm sorry for the trouble it might have caused.

I cloned your repository and did some tests, unfortunately it contains a bug that was fixed by the line you removed: when you change the crate's version, the documentation link is not correctly updated.

I assembled a patch that should fix the original issue while not breaking anything, feel free to test it and integrate it in your branch:

diff --git a/app/routes/crate/version.js b/app/routes/crate/version.js
index 79835c38..6c8189e4 100644
--- a/app/routes/crate/version.js
+++ b/app/routes/crate/version.js
@@ -41,13 +41,16 @@ export default Route.extend({
         };
 
         const fetchCrateDocumentation = () => {
-            if (!crate.get('documentation')) {
-                let crateName = crate.get('name');
-                let crateVersion = params.version_num;
+            if (!crate.get('documentation') ||
+                crate.get('documentation').substr(0, 16) === 'https://docs.rs/') {
+                const crateName = crate.get('name');
+                const crateVersion = params.version_num;
+                const docsURL = `https://docs.rs/${crateName}/${crateVersion}/`;
+                const docsPlatform = (crate.get('documentation') || docsURL).slice(16).split('/').slice(2).join('/');
                 this.get('ajax').request(`https://docs.rs/crate/${crateName}/${crateVersion}/builds.json`)
                     .then((r) => {
                         if (r.length > 0 && r[0].build_status === true) {
-                            crate.set('documentation', `https://docs.rs/${crateName}/${crateVersion}/`);
+                            crate.set('documentation', `${docsURL}${docsPlatform}`);
                         }
                     });
             }

[DoumanAsh]

@kureuil I'm a bit confused.
Is it this bug coming from the fact that after initial update of link when crate had none, on version change front-end will not be able to detect that link is needed due to it being previously added?

Your solution will make no sense for #1206 as you still overwrote author's link.

A better solution would be to clear out crate.get('documentation').
Is it possible to do on version change of crate?

Generally I would prefer remove this docs.rs 'integration' from front-end and moving it to back-end itself.
This override should be done on server side without fetching extra resources from docs.rs.

P.s. another thing it would be better to not update original documentation and instead use some another attribute to store new link (in order to not mess UI data and back-end data)

P.s. 2 Is crate variable a model from https://github.com/rust-lang/crates.io/blob/master/app/models/crate.js ?

[carols10cents]

This override should be done on server side without fetching extra resources from docs.rs.

We need to check with docs.rs to make sure there is a successful build of documentation before we link to it. I'm ok moving that check from the client side to the server side, but the check is important because we don't want to link to docs that are broken or missing.

[DoumanAsh]

@carols10cents I believe it is author responsibility to provide valid link to documentation.
It is not up to crates.io to decide which link is valid or not, especially in such bad manner when author is not able to link docs for particular platform(i.e. Windows).

There is though another option: Provide extra link to docs.rs if author specified own link.

[carols10cents]

There is though another option: Provide extra link to docs.rs if author specified own link.

The intent of this feature was to only link to docs.rs if the author has not specified a link.

[DoumanAsh]

The intent of this feature was to only link to docs.rs if the author has not specified a link.

Yes, that's why I removed this check crate.get('documentation').substr(0, 16) === 'https://docs.rs/') which currently overwrites author's link if it is on docs.rs

But according to @kureuil it is necessary to prevent UI bug on switching versions.

[tchak]

Could you please use camelcase as the rest of variables do in the javascript project (we should have a linter roule for this, I know)

[DoumanAsh]

I added to model a separate attribute to store original link to docs.
This way hopefully it is not gonna get overwritten now.

Since I cannot install sass, i'd appreciate someone to check it out to be sure

[DoumanAsh]

@carols10cents ping?

[nagisa]

Current overwrite from https://docs.rs/<crate>/ to https://docs.rs/<crate>/<version> is extremely useful and avoids churn of having to update the link in Cargo.toml every release.

[retep998]

@nagisa But it also currently overwrites from https://docs.rs/<crate>/<version>/<platform> to just https://docs.rs/<crate>/<version> which means people following the documentation link are dumped at a useless page with no documentation.

I would be okay with a modification that makes it preserve the <platform> part when it overwrites the link.

[nagisa]

Yeah, to me it seems like involving the url crate would be ideal here. So we could properly check for domain used and swap out the version number if that component looks like a version number without affecting all other path components.

[DoumanAsh]

@nagisa From what I understand current behavior is not intentional but consequence of bad implementation.

It could be obviously done, but it makes sense to do it on server side, rather than doing it each time on client side.

[DoumanAsh]

Review plz?

[carols10cents]

but it makes sense to do it on server side,

I would accept a PR to do docs.rs documentation URL detection/manipulation on the server side. Because you seem to think that would be a better way as well, I'm going to close this PR. Thanks!