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

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

Closed

Conversation

@benjaoming
Copy link
Contributor

@benjaoming 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
Copy link
Member

@humitos humitos commented Dec 8, 2017

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

https://github.com/rtfd/readthedocs.org/blob/c8a72f5b351644f8e0bb6c4b6ad55a4ba0edba7d/readthedocs/builds/models.py#L252

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

@humitos
Copy link
Member

@humitos humitos commented Dec 8, 2017

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

https://github.com/rtfd/readthedocs.org/blob/c8a72f5b351644f8e0bb6c4b6ad55a4ba0edba7d/readthedocs/builds/models.py#L139-L145

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
Copy link
Member

@humitos 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
Copy link
Contributor Author

@benjaoming 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
Copy link
Member

@humitos 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
Copy link
Contributor Author

@benjaoming 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
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants