Allow permalink template to have underscores #5572

Merged
merged 7 commits into from Nov 14, 2016

Projects

None yet

4 participants

@parkr
Member
parkr commented Nov 10, 2016

Fixes #5330.

/cc @qpwo

@parkr parkr added bug fix labels Nov 10, 2016
@parkr parkr added this to the 3.3.1 milestone Nov 10, 2016
@parkr
Member
parkr commented Nov 10, 2016
@pnn pnn was assigned by jekyllbot Nov 10, 2016
@parkr
Member
parkr commented Nov 10, 2016

@jekyll/core Please review!

@pnn
Member
pnn commented Nov 11, 2016

nice, i've been waiting for this one for ages. LGTM

@pnn
Member
pnn commented Nov 11, 2016

@parkr actually, what good does it do to have a team leader assigned to a PR when they can't even approve it? it seems weird to me

(also, are there any ways to contact you privately?)

parkr added some commits Nov 10, 2016
@parkr parkr Add failing test for permalink templates with trailing underscores f27eb77
@parkr parkr Whoops, an *actually useful* failing test. 9f8f031
@parkr parkr URL#generate_url_from_drop: be smarter about replacing *just* the keys 347651e
@parkr parkr Add useful comment. d50ef0e
@parkr parkr Fix fmt error. ff012e7
@parkr parkr Dates are _the worst_
cb67240
lib/jekyll/url.rb
@@ -84,17 +84,36 @@ def generate_url_from_hash(template)
end
end
+ # We include underscores in keys to allow for 'i_month' and so forth.
+ # This poses a problem for keys which are followed by and underscore
@ashmaroli
ashmaroli Nov 12, 2016 Contributor

small typo: it should be an underscore instead of and underscore

@parkr
parkr Nov 14, 2016 Member

Thank you! Updated.

lib/jekyll/url.rb
+ # This poses a problem for keys which are followed by and underscore
+ # but the underscore is not part of the key, e.g. '/:month_:day'.
+ # That should be :month and :day, but our key extraction regexp isn't
+ # smart enough to know that so we have to make it an explicit
@ashmaroli
ashmaroli Nov 12, 2016 Contributor

personal nit: it would be better to have a , (comma) between ..that and so.. 😃

- "".freeze
- else
- self.class.escape_path(@placeholders[key])
+ pool = possible_keys(match.sub(":".freeze, "".freeze))
@ashmaroli
ashmaroli Nov 12, 2016 Contributor

casual query: why do we have to explicitly freeze the strings here? 🤔

@parkr
parkr Nov 14, 2016 Member

It's a virtual machine optimization.

@parkr parkr Fix typo.
d3e387d
@parkr
Member
parkr commented Nov 14, 2016

@parkr actually, what good does it do to have a team leader assigned to a PR when they can't even approve it? it seems weird to me

@fene You can approve! If you go to the Files tab, I believe you can approve this. There are some weird nuances with

(also, are there any ways to contact you privately?)

Yes! By email, listed on my GitHub profile. I don't check it very often as I get a lot of spam. Did you email me? I'll check it now!

@parkr parkr changed the title from Add failing test for permalink templates with trailing underscores to Allow permalink template to have underscores Nov 14, 2016
@parkr
Member
parkr commented Nov 14, 2016

@jekyllbot: merge +bug

@jekyllbot jekyllbot merged commit 88a338d into master Nov 14, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jekyllbot jekyllbot added bug fix labels Nov 14, 2016
@jekyllbot jekyllbot deleted the fix-underscore-in-permalink branch Nov 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment