Edge guides don't properly link to the current GitHub tree #8221

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
Contributor

agrobbin commented Nov 14, 2012

WIthout the full commit hash, GitHub responds with a 404 when clicking the edge guides "master branch" link. I would have committed this directly to lifo/docrails, but it does include code-related changes in generator.rb, so I thought it was warranted to put it here first.

Owner

rafaelfranca commented Nov 14, 2012

cc @fxn

Member

steveklabnik commented Nov 14, 2012

👍

@@ -85,6 +85,7 @@ def set_flags_from_environment
@all = ENV['ALL'] == '1'
@kindle = ENV['KINDLE'] == '1'
@version = ENV['RAILS_VERSION'] || `git rev-parse --short HEAD`.chomp
+ @full_version = ENV['RAILS_VERSION'] || `git rev-parse HEAD`.chomp
@vijaydev

vijaydev Nov 15, 2012

Member

if the short SHA is sent in the ENV variable, this is still a problem right? I 'd suggest to remove the ENV check for the full version.

@agrobbin

agrobbin Nov 15, 2012

Contributor

Is there any time that ENV['rails_version'] would be passed when it wouldn't be a branch or tag name? Or rather, when it would be a short commit hash?

@vijaydev

vijaydev Nov 15, 2012

Member

I'm not sure how it is set up on the docs server. @fxn can tell.

Owner

fxn commented Nov 15, 2012

Oh, interesting. I am sure that worked in the past.

We have two use cases here.

In the stable docs, the version is the git tag, eg "v3.2.9". The shell script that detects there is a new stable version passes the tag name to the stable docs generator via RAILS_VERSION, and it is used both in the title (within parens), and in the link of the preamble.

In the edge docs, I believe we could also pass the SHA1 in RAILS_VERSION and remove the backticks. To make it uniform. I would update the scripts in the server.

Then, if @edge is true a simple @version[0, 7] within parens would suffice for the title.

Contributor

agrobbin commented Nov 18, 2012

How would you recommend I proceed with this pull request?

@agrobbin agrobbin closed this Nov 18, 2012

@agrobbin agrobbin reopened this Nov 18, 2012

@fxn fxn closed this in 0adcf6d Dec 7, 2012

Owner

fxn commented Dec 7, 2012

Hi, finally I have done this myself because it needed synchronization with generation scripts in the docs server. Thanks for noticing this bug.

Contributor

agrobbin commented Dec 7, 2012

No problem, thanks for fixing it!

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