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

Fix broken link to Upgrading Ruby on Rails Guide #16545

Merged
merged 1 commit into from
Aug 18, 2014
Merged

Conversation

jonatack
Copy link
Contributor

in the 4.2 Release Notes [skip ci]

@@ -18,7 +18,7 @@ coverage before going in. You should also first upgrade to Rails 4.1 in case you
haven't and make sure your application still runs as expected before attempting
an update to Rails 4.2. A list of things to watch out for when upgrading is
available in the
[Upgrading Ruby on Rails](upgrading_ruby_on_rails.html#upgrading-from-rails-4-1-to-rails-4-2)
[Upgrading Ruby on Rails](upgrading_ruby_on_rails.md#upgrading-from-rails-41-to-rails-42)
Copy link
Member

Choose a reason for hiding this comment

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

It should be "upgrading_ruby_on_rails.html#upgrading-from-rails-4.1-to-rails-4.2"

http://edgeguides.rubyonrails.org/upgrading_ruby_on_rails.html#upgrading-from-rails-4.1-to-rails-4.2

in the 4.2 Release Notes [skip ci]

Change to link suggested by @zzak [skip ci]
@jonatack
Copy link
Contributor Author

Thanks @zzak, squashed.

zzak pushed a commit that referenced this pull request Aug 18, 2014
Fix broken link to Upgrading Ruby on Rails Guide [ci skip]
@zzak zzak merged commit 321d4cb into rails:master Aug 18, 2014
@zzak
Copy link
Member

zzak commented Aug 18, 2014

@jonatack Thanks!

@jonatack jonatack deleted the patch-9 branch August 18, 2014 22:33
@chancancode
Copy link
Member

@fxn @zzak @robin850 @jonatack Actually, this seems to be a regression in redcarpet or something else the guides generator code. This format was used for all the previous release notes and it as you can see it works on the live site. Any idea what caused this?

In any case, we should probably revert this and fix the real problem. I suspect we there are many other links that expects the same anchors in our guides and elsewhere on the interwebz.

@zzak
Copy link
Member

zzak commented Aug 19, 2014

@chancancode Good point! I only checked the one file to make sure the link worked on edgeguides.

@JuanitoFatas
Copy link
Contributor

@chancancode Probably due to my commit: 699babe in this PR #15242. But I need this to make my chinese translations work. I'll look into this.

@fxn
Copy link
Member

fxn commented Aug 20, 2014

Back to square one after 597a666.

That commit restores the link in addition to revert the original change.

@zzak
Copy link
Member

zzak commented Aug 20, 2014

Revert commits count too! :trollface:

@robin850
Copy link
Member

Sorry if I'm understanding it wrong but actually we are removing non-ASCII characters just to remove special characters (like &, | or *). This is a bit more code but what do you think about:

diff --git a/guides/rails_guides/markdown.rb b/guides/rails_guides/markdown.rb
index b422a80..1703506 100644
--- a/guides/rails_guides/markdown.rb
+++ b/guides/rails_guides/markdown.rb
@@ -47,7 +47,12 @@ module RailsGuides
       end

       def dom_id_text(text)
-        text.downcase.gsub(/\?/, '-questionmark').gsub(/!/, '-bang').gsub(/[^a-z0-9]+/, ' ').strip.gsub(/\s+/, '-')
+        escaped_chars = Regexp.escape('\\/`*_{}[]()#+-.!:,;|&<>^~=\'"')
+
+        text.downcase.gsub(/\?/, '-questionmark')
+                     .gsub(/!/, '-bang')
+                     .gsub(/[#{escaped_chars}]+/, ' ').strip
+                     .gsub(/\s+/, '-')
       end

       def engine

The output folder generate guides with and without this patch will be the same. @JuanitoFatas: Could you please test that in your fork ? If it looks good to you guys, feel free to commit it. :-)

@JuanitoFatas
Copy link
Contributor

@robin850 I was trying to do things like this but thought I may not consider all cases. I'll test it tomorrow and let you know, Thanks!!!!! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants