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

Strip well-known version component origin/ from remote version #3377

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@benjaoming
Contributor

benjaoming commented Dec 7, 2017

Fixes #3203

As far as I can tell all "Edit on Github" buttons have broken URLs when they refer to branch names.

Examples:
http://django-wiki.readthedocs.io/en/master/
http://discretize.simpeg.xyz/en/master/

Somehow, if it's just a default latest slug pointing to the master branch, then there's no appending of origin/. There's some stuff going on in vcs_support.backends.git that I can't really say might have a cause of that... and that perhaps the fix should be placed there. Anyways, the generation of a URL is view logic, so I kind of prefer my simple fix, putting the logic where the link is generated.

https://github.com/rtfd/readthedocs.org/blob/master/readthedocs/vcs_support/backends/git.py#L127

Example of non-affected page:
http://django-statsd.readthedocs.io/en/latest/

@humitos

This comment has been minimized.

Member

humitos commented Dec 8, 2017

I suppose the logic for this should be added/fixed in the get_github_url method:

def get_github_url(

There version=self.commit_name is passed to GITHUB_URL template.

@humitos

This comment has been minimized.

Member

humitos commented Dec 8, 2017

Also, seems that the commit_name property is not working as it says it should:

if self.type in (BRANCH, TAG):
# If this version is a branch or a tag, the verbose_name will
# contain the actual name. We cannot use identifier as this might
# include the "origin/..." part in the case of a branch. A tag
# would contain the hash in identifier, which is not as pretty as
# the actual tag name.
return self.verbose_name

I think that's the right place to attack for this fix and I think we will need a test for this also.

@benjaoming benjaoming closed this Dec 8, 2017

@humitos

This comment has been minimized.

Member

humitos commented Dec 10, 2017

Hey @benjaoming. I noticed you closed this issue. I hope it wasn’t because I might have been a bit short with you. My bad. I think the bug is not solved and we need to fix it. Would you like to continue working with me on get this fixed?

@benjaoming

This comment has been minimized.

Contributor

benjaoming commented Dec 10, 2017

I saw that my fix was misjudged by miles :) With what I have of bandwidth, it's going to take me a considerable effort to get on top of it (bootstrapping dev environment + testing).. so I hope someone else will do it.. sorry though for the failed "lucky punch" PR :D

@humitos

This comment has been minimized.

Member

humitos commented Dec 21, 2017

Hi @benjaoming! My PR was already merged, so I think it will deployed next week. I hope it fix you issue as I expect.

Again, I'm sorry if I was rude with you, it wasn't my intention at all.

@benjaoming

This comment has been minimized.

Contributor

benjaoming commented Dec 22, 2017

Nono, not at all, I looked over what I was doing code-wise -- and that was what I was referring to which was misjudged by miles: I misjudged by miles and opened a PR that was way off :) Thanks for fixing this, I'm right now waiting for a build just to confirm that things are working 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment