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

Use verbose_name for stable tag commit name #3142

Closed
wants to merge 1 commit into from

Conversation

Peque
Copy link

@Peque Peque commented Oct 4, 2017

It is clearer, nicer and human-readable.

It may fix #2563 (see my comments in the issue to understand what is happening).

@Peque
Copy link
Author

Peque commented Oct 18, 2017

@ericholscher @agjohnson Friendly ping. 😇

@agjohnson
Copy link
Contributor

Though I feel the hash is more of a canonical link to GitHub, and perhaps we just need to do something to get the correct hash to link to, this does seem like a reasonable approach. This would probably need some QA though, as it's changing a pretty fundamental, and not particularly well maintained portion of code.

@Peque
Copy link
Author

Peque commented Oct 20, 2017

@agjohnson The hash is correct. That means, it is the hash that references the tag in Git (i.e.: you can git checkout <tag_hash>). GitHub does not recognize it though, so the generated link is broken.

I think the only options would be to either:

  • Hope for GitHub to fix/implement this some day (that day may never come).
  • Use an alternative hash, which could be the commit hash instead of the tag hash. This is recognized by GitHub, but I do not think it is correct to link to a commit when we want to refer to a tag. Also, I think this would require deeper or more complex changes than those proposed in this pull request.
  • Use the canonical name of the tag, which is readable and will generate shorter and more human-friendly URLs (which is my proposed approach).

This solution might also be the best to keep a more consistent GitHub URL generation. Notice that old stable releases all point to the human-readable tag name (i.e.: mayor.minor.patch version), the latest version points to a readable name as well (i.e.: master branch), but only the stable version points to a non-readable hash (i.e.: ugly abcdef0123456789...).

Agree with the QA. The change is very small, but of course requires some checking/validation on your side.

Hope you can find some time to do it soon, as currently the GitHub links generated from the stable release are broken (which guess it could be considered as a bug, even if the issue is not labeled as so). 😊

@techtonik
Copy link
Contributor

#3238 fixes stable links. As for underlying code, it could be better refactored to always carry commit id in the model instead of juggling it with STABLE and LATEST markers.

@ericholscher
Copy link
Member

Thanks for this PR. I have flagged it for review, because it looks quite simple. I'm not sure of the downstream effects it might have though.

@stsewd
Copy link
Member

stsewd commented May 30, 2018

The original issue was resolved by #4052. I'm not sure how this would impact on the UI (I think this is used in the versions list).

@Peque
Copy link
Author

Peque commented May 31, 2018

@ericholscher Thanks for reconsidering this. 😊

@stsewd Even if #2563 is resolved I think this could still be interesting. After all, wouldn't it be better to use the actual latest stable tag name when linking to the original source code than to use a hash value? It is definitely more readable and would work just as well.

Note that:

  • Old stable releases like x.y.z all point to the human-readable tag name (i.e.: x.y.z).
  • The latest version points to a readable name as well (i.e.: master).
  • Only the stable version points to a non-readable hash (i.e.: ugly abcdef0123456789...).

So, apart from readability, it could be considered for consistency with the rest of built versions and generated source code links.

@humitos
Copy link
Member

humitos commented May 31, 2018

I agree with @Peque on this.

My only concern here, as @agjohnson described previously is, where else is this code used? I'd like to see the verbose name on the Build listing and other places, but I wouldn't like to break GitHub URLs because of this.

So, I'd like if someone else can do some QA locally to see if this don't break. At least we need to manually test github/gitlab/bitbucket URLs for View/Edit are properly generated for stable branch. Also, is it possible to have some tests written for this generated URLs if we don't have already (I suppose we don't have, because otherwise they would fail already)?

Summarizing, I'd say that after that QA we can merge this PR.

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

I did a quick test of this PR by pulling it down and rebase it against master and I found that "Edit on Gitlab" link breaks for stable version. The URL generated is like https://gitlab.com/humitos/rtd-project-a/blob/stable/index.rst and it gives 404

The same QA test using the latest master code of RTD does work properly.

Because of this, I'm marking this as "Request changes".

@humitos humitos added PR: work in progress Pull request is not ready for full review and removed PR: ready for review labels May 31, 2018
@ericholscher
Copy link
Member

Closing this, as it hasn't been updated in a long time. Feel free to reopen with requested changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: work in progress Pull request is not ready for full review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

stable build process appears to pick up correct commit, but publishes a stale one
6 participants