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

Add latin mode to slugify #6509

Merged
merged 9 commits into from Nov 3, 2017

Conversation

Projects
None yet
7 participants
@alextsui05
Contributor

alextsui05 commented Nov 1, 2017

This is in response to #4623

To summarize the discussion in the above issue, trying to slugify strings with Swedish letters in them (accented characters in general) produced unexpected output. ascii mode does the job but drops accented characters, which is probably undesirable to some users. So we add latin mode, which will transliterate accented characters to the ASCII equivalent.

Example:

{{ 'kvalité, då, äta, öl' | slugify: 'ascii' }} # -> kvalit-d-ta-l
{{ 'kvalité, då, äta, öl' | slugify: 'latin' }} # -> kvalite-da-ata-ol

Changes

  • Add a new latin mode to be used as an option for #slugify
  • Add runtime dependency on i18n gem to perform the transliteration
  • Update the documentation to demonstrate latin mode, and also ascii mode, which existed but was not documented

Documentation screenshots

vmplayer_2017-11-02_18-04-38

vmplayer_2017-11-02_18-05-00

@DirtyF DirtyF requested a review from jekyll/build Nov 1, 2017

@DirtyF DirtyF added the enhancement label Nov 1, 2017

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 1, 2017

Member

Hello @alextsui05 from a first-pass, it looks like you're calling I18n::transliterate instead of ActiveSupport::Inflector::transliterate. Do correct me if I'm wrong, but I like using I18n directly so that we need not pull in the entire ActiveSupport library but can instead depend on gem "i18n" directly.

That said, from the failing CI logs, it looks like the intended results are being delivered, but for the maintainers to accept this, they'd like to have the tests passing and have this change documented because suddenly users of the :ascii mode are going to find their slugs different.. ergo, a broken-link and the 404 fiasco..

Member

ashmaroli commented Nov 1, 2017

Hello @alextsui05 from a first-pass, it looks like you're calling I18n::transliterate instead of ActiveSupport::Inflector::transliterate. Do correct me if I'm wrong, but I like using I18n directly so that we need not pull in the entire ActiveSupport library but can instead depend on gem "i18n" directly.

That said, from the failing CI logs, it looks like the intended results are being delivered, but for the maintainers to accept this, they'd like to have the tests passing and have this change documented because suddenly users of the :ascii mode are going to find their slugs different.. ergo, a broken-link and the 404 fiasco..

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Nov 1, 2017

Member

because suddenly users of the :ascii mode are going to find their slugs different.. ergo, a broken-link and the 404 fiasco..

Which makes this a major change rather than an enhancement, correct?

Member

pathawks commented Nov 1, 2017

because suddenly users of the :ascii mode are going to find their slugs different.. ergo, a broken-link and the 404 fiasco..

Which makes this a major change rather than an enhancement, correct?

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 1, 2017

Member

Which makes this a major change rather than an enhancement, correct?

As of now, yes, but if the author proposes a new mode altogether, then it can still be an enhancement..

Member

ashmaroli commented Nov 1, 2017

Which makes this a major change rather than an enhancement, correct?

As of now, yes, but if the author proposes a new mode altogether, then it can still be an enhancement..

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 1, 2017

Member

@alextsui05 You'll need to make a couple of changes:

  • Jekyll uses double-quotes everywhere
  • You may want to consider introducing a new slugify mode so that existing :ascii mode slugs do not break. (I'm thinking maybe, :ascii_plus, is nice, but its open to your creativity..)
Member

ashmaroli commented Nov 1, 2017

@alextsui05 You'll need to make a couple of changes:

  • Jekyll uses double-quotes everywhere
  • You may want to consider introducing a new slugify mode so that existing :ascii mode slugs do not break. (I'm thinking maybe, :ascii_plus, is nice, but its open to your creativity..)
Show outdated Hide outdated Gemfile Outdated
@alextsui05

This comment has been minimized.

Show comment
Hide comment
@alextsui05

alextsui05 Nov 1, 2017

Contributor

I realized active_support is a heavy requirement, so I took @ashmaroli 's suggestion and updated the code to use i18n (+ updated the test).

The more I think about it, maybe it would be better for compatibility + clearer to define a new mode that is more explicit about what it does -- maybe slugify: 'latin' gives a good hint to the user.

I18n.transliterate('á, à, ç, é, è, í, ï, ó, ò, ú, ü, n·h, s·h.')
 => "a, a, c, e, e, i, i, o, o, u, u, n?h, s?h."

I'll make another pass at the code at the end of the day to address the style issues too. Thanks for the help.

Contributor

alextsui05 commented Nov 1, 2017

I realized active_support is a heavy requirement, so I took @ashmaroli 's suggestion and updated the code to use i18n (+ updated the test).

The more I think about it, maybe it would be better for compatibility + clearer to define a new mode that is more explicit about what it does -- maybe slugify: 'latin' gives a good hint to the user.

I18n.transliterate('á, à, ç, é, è, í, ï, ó, ò, ú, ü, n·h, s·h.')
 => "a, a, c, e, e, i, i, o, o, u, u, n?h, s?h."

I'll make another pass at the code at the end of the day to address the style issues too. Thanks for the help.

@alextsui05 alextsui05 changed the title from Add transliterate step on slugify with ascii mode to Add latin mode to slugify Nov 2, 2017

@alextsui05

This comment has been minimized.

Show comment
Hide comment
@alextsui05

alextsui05 Nov 2, 2017

Contributor

I updated the PR to make it clear that we are adding a new latin mode rather than modifying the ascii mode. The CI tests are still winding down but I believe style issues are all ironed out.

Contributor

alextsui05 commented Nov 2, 2017

I updated the PR to make it clear that we are adding a new latin mode rather than modifying the ascii mode. The CI tests are still winding down but I believe style issues are all ironed out.

Show outdated Hide outdated lib/jekyll/utils.rb Outdated
@oe

oe approved these changes Nov 2, 2017

LGTM but i like @ashmaroli's suggestion

@DirtyF

DirtyF approved these changes Nov 2, 2017

La France 🇫🇷 🥖 🧀 thanks you for this new latin mode @alextsui05 ❤️❤️ ❤️

@alextsui05

This comment has been minimized.

Show comment
Hide comment
@alextsui05

alextsui05 Nov 2, 2017

Contributor

I took @ashmaroli 's advice and simplified the switch statement.

Thanks all for the reviews and help!

Contributor

alextsui05 commented Nov 2, 2017

I took @ashmaroli 's advice and simplified the switch statement.

Thanks all for the reviews and help!

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 2, 2017

Member

@alextsui05 Thank you very much! This is already an awesome First-Contribution to Jekyll! 🎉
Congratulations in advance! 😃

Wanna take it to a higher level?
If yes, you could document this new mode by editing this section and table right above


On a side-note, if you've the time, you can open a new PR documenting the ascii mode

Member

ashmaroli commented Nov 2, 2017

@alextsui05 Thank you very much! This is already an awesome First-Contribution to Jekyll! 🎉
Congratulations in advance! 😃

Wanna take it to a higher level?
If yes, you could document this new mode by editing this section and table right above


On a side-note, if you've the time, you can open a new PR documenting the ascii mode

@mattr-

This is looking great! Thanks for your work on this!

I do have two small changes that I request you make before I feel this is ready to merge.

Show outdated Hide outdated lib/jekyll/utils.rb Outdated
# Strip according to the mode
slug = string.gsub(re, "-")
slug = replace_character_sequence_with_hyphen(string, :mode => mode)

This comment has been minimized.

@mattr-

mattr- Nov 2, 2017

Member

Using the symbol syntax would be better here, so that you're consistent with the rest of the codebase.

mode: mode
@mattr-

mattr- Nov 2, 2017

Member

Using the symbol syntax would be better here, so that you're consistent with the rest of the codebase.

mode: mode

This comment has been minimized.

@alextsui05

alextsui05 Nov 2, 2017

Contributor

@mattr- Another style error in the build came up here as well, something about using hash rocket syntax, so I wrote it this way.

@alextsui05

alextsui05 Nov 2, 2017

Contributor

@mattr- Another style error in the build came up here as well, something about using hash rocket syntax, so I wrote it this way.

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Nov 2, 2017

Member

Related to my above review comment, I would prefer that the documentation work for the latin filter be done as part of this PR as well. The documentation for the ascii mode isn't required since it's an existing part of the codebase.

Member

mattr- commented Nov 2, 2017

Related to my above review comment, I would prefer that the documentation work for the latin filter be done as part of this PR as well. The documentation for the ascii mode isn't required since it's an existing part of the codebase.

@alextsui05

This comment has been minimized.

Show comment
Hide comment
@alextsui05

alextsui05 Nov 2, 2017

Contributor

@mattr- Thanks, I'll update the documentation in the docs/ subdirectory after the work day is over today. Also, I responded to the review comments about what I ran into, so if you could advise me on what to do there, I'll also take care of those changes.

Contributor

alextsui05 commented Nov 2, 2017

@mattr- Thanks, I'll update the documentation in the docs/ subdirectory after the work day is over today. Also, I responded to the review comments about what I ran into, so if you could advise me on what to do there, I'll also take care of those changes.

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Nov 2, 2017

Member

@alextsui05 If making those changes would cause the build to fail, then let's not do those right now. I think that's on us as maintainers to fix those. 😃

Looking forward to seeing the new documentation and getting this merged soon! Thanks!
bitmoji

Member

mattr- commented Nov 2, 2017

@alextsui05 If making those changes would cause the build to fail, then let's not do those right now. I think that's on us as maintainers to fix those. 😃

Looking forward to seeing the new documentation and getting this merged soon! Thanks!
bitmoji

@ashmaroli ashmaroli added this to the v3.7.0 milestone Nov 2, 2017

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 3, 2017

Member

I think its best that you make only changes related to the latin mode in this PR.. everything else in a separate PR..

Member

ashmaroli commented Nov 3, 2017

I think its best that you make only changes related to the latin mode in this PR.. everything else in a separate PR..

@alextsui05

This comment has been minimized.

Show comment
Hide comment
@alextsui05

alextsui05 Nov 3, 2017

Contributor

@mattr- I just pushed a documentation update that includes a description of the new latin mode. While I was at it, I also added a description for ascii mode that @ashmaroli mentioned -- it was existing but was not actually in the documentation.

I posted screenshots on the main body of the PR for the sections that changed. Once that's been finalized, I'll also update the table in the link @ashmaroli posted in an earlier comment.

Contributor

alextsui05 commented Nov 3, 2017

@mattr- I just pushed a documentation update that includes a description of the new latin mode. While I was at it, I also added a description for ascii mode that @ashmaroli mentioned -- it was existing but was not actually in the documentation.

I posted screenshots on the main body of the PR for the sections that changed. Once that's been finalized, I'll also update the table in the link @ashmaroli posted in an earlier comment.

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 3, 2017

Member

I'll also update the table in the link

@alextsui05 Don't worry about it. The "table" has already been updated with your latest commit..

Member

ashmaroli commented Nov 3, 2017

I'll also update the table in the link

@alextsui05 Don't worry about it. The "table" has already been updated with your latest commit..

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 3, 2017

Member

it was existing but was not actually in the documentation.

again, that should be handled in a separate PR..

Member

ashmaroli commented Nov 3, 2017

it was existing but was not actually in the documentation.

again, that should be handled in a separate PR..

@alextsui05

This comment has been minimized.

Show comment
Hide comment
@alextsui05

alextsui05 Nov 3, 2017

Contributor

Okay, let me split out the ascii part of the documentation into a separate commit and put it in a new PR.

Contributor

alextsui05 commented Nov 3, 2017

Okay, let me split out the ascii part of the documentation into a separate commit and put it in a new PR.

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Nov 3, 2017

Member

Okay, let me split out the ascii part of the documentation into a separate commit and put it in a new PR.

Please don't. That's just churn for no good reason.

Member

mattr- commented Nov 3, 2017

Okay, let me split out the ascii part of the documentation into a separate commit and put it in a new PR.

Please don't. That's just churn for no good reason.

@mattr-

mattr- approved these changes Nov 3, 2017

@pathawks

Thanks for all the work you put into this! Really appreciate all the polish 👍

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 3, 2017

Member

That's just churn for no good reason.

¯\_(ツ)_/¯

😆

Member

ashmaroli commented Nov 3, 2017

That's just churn for no good reason.

¯\_(ツ)_/¯

😆

@alextsui05

This comment has been minimized.

Show comment
Hide comment
@alextsui05

alextsui05 Nov 3, 2017

Contributor

On a side-note, if you've the time, you can open a new PR documenting the ascii mode

@ashmaroli Sorry, I remembered the part about documenting ascii mode but forgot the part about in a new PR (this gets me at work, too). I'll keep it in mind for the next PR!

Contributor

alextsui05 commented Nov 3, 2017

On a side-note, if you've the time, you can open a new PR documenting the ascii mode

@ashmaroli Sorry, I remembered the part about documenting ascii mode but forgot the part about in a new PR (this gets me at work, too). I'll keep it in mind for the next PR!

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Nov 3, 2017

Member

I see no reason for the appveyor build to fail since all I did was merge in master so that we can merge this, so away we go!

@jekyllbot: merge +minor

@alextsui05 Thank you again for making the changes we've requested and congratulations on your first contribution to Jekyll!
bitmoji

Member

mattr- commented Nov 3, 2017

I see no reason for the appveyor build to fail since all I did was merge in master so that we can merge this, so away we go!

@jekyllbot: merge +minor

@alextsui05 Thank you again for making the changes we've requested and congratulations on your first contribution to Jekyll!
bitmoji

@jekyllbot jekyllbot merged commit 93e3eb0 into jekyll:master Nov 3, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@DirtyF DirtyF referenced this pull request Nov 12, 2017

Closed

slugify option 'latin' unreleased #6549

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