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 strip_index filter #6075

Merged
merged 5 commits into from Jun 15, 2017

Conversation

Projects
None yet
7 participants
@benbalter
Contributor

benbalter commented May 15, 2017

jekyll/jekyll-sitemap#170 made me realize that in several of Jekyll's core plugins we use the pattern {{ file.path | replace:'/index.html','/' }}.

That filter works, but is (A) easy to forget, and (B) is a blind replace, not a replace of the a trailing /index.html.

This PR adds a quick strip_index filter (very up for a better name, maybe pretty_permalink?), to implement the above logic as a native URL filter.

Thoughts?

benbalter added some commits May 15, 2017

Show outdated Hide outdated test/test_filters.rb
context "strip_index filter" do
should "strip trailing /index.html" do
assert_equal `/foo/`, @filter.strip_index('/foo/index.html')
end

This comment has been minimized.

@ashmaroli

ashmaroli May 15, 2017

Member

Why are we asserting /foo/ enclosed in back ticks instead of quotes?

@ashmaroli

ashmaroli May 15, 2017

Member

Why are we asserting /foo/ enclosed in back ticks instead of quotes?

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks May 15, 2017

Member

Is there a time when “we” (plugins or themes) would want to use the URL filters without stripping index.html?

Member

pathawks commented May 15, 2017

Is there a time when “we” (plugins or themes) would want to use the URL filters without stripping index.html?

Show outdated Hide outdated lib/jekyll/filters/url_filters.rb
# Returns a URL with the trailing `/index.html` removed
def strip_index(input)
return if input.nil?
input.sub(%r!/\.index\.html?$!, "/")

This comment has been minimized.

@ashmaroli

ashmaroli May 15, 2017

Member

Error in the regex here:

- input.sub(%r!/\.index\.html?$!, "/")
+ input.sub(%r!/index\.html?$!, "/")
@ashmaroli

ashmaroli May 15, 2017

Member

Error in the regex here:

- input.sub(%r!/\.index\.html?$!, "/")
+ input.sub(%r!/index\.html?$!, "/")
@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli May 15, 2017

Member

Is there a time when “we” (plugins or themes) would want to use the URL filters without stripping index.html?

From the test case included, I'm guessing:

/foo/index.html#bar
Member

ashmaroli commented May 15, 2017

Is there a time when “we” (plugins or themes) would want to use the URL filters without stripping index.html?

From the test case included, I'm guessing:

/foo/index.html#bar
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 19, 2017

Member

LGTM once the tests are passing!

Member

parkr commented May 19, 2017

LGTM once the tests are passing!

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter May 22, 2017

Contributor

Is there a time when “we” (plugins or themes) would want to use the URL filters without stripping index.html?

I'm thinking that may be a better approach. Any reason not to?

@parkr mentioned the edge case where servers don't support indexes, but this should be the default in 99% of environments.

Contributor

benbalter commented May 22, 2017

Is there a time when “we” (plugins or themes) would want to use the URL filters without stripping index.html?

I'm thinking that may be a better approach. Any reason not to?

@parkr mentioned the edge case where servers don't support indexes, but this should be the default in 99% of environments.

@parkr

parkr approved these changes Jun 14, 2017

@parkr parkr referenced this pull request Jun 14, 2017

Closed

Release Jekyll v3.5 #6074

@oe

This comment has been minimized.

Show comment
Hide comment
@oe

oe Jun 15, 2017

Member

strip_index sounds fine to me, it describes what it does pretty well

Member

oe commented Jun 15, 2017

strip_index sounds fine to me, it describes what it does pretty well

@oe

oe approved these changes Jun 15, 2017

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Jun 15, 2017

Contributor

Do we want to go this route or the route of automatically doing this in the existing URL filters?

Contributor

benbalter commented Jun 15, 2017

Do we want to go this route or the route of automatically doing this in the existing URL filters?

@oe

This comment has been minimized.

Show comment
Hide comment
@oe

oe Jun 15, 2017

Member

@benbalter i think the latter would be a major change? considering that we want to get this in 3.5, let's go with this for now

Member

oe commented Jun 15, 2017

@benbalter i think the latter would be a major change? considering that we want to get this in 3.5, let's go with this for now

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Jun 15, 2017

Member

considering that we want to get this in 3.5, let's go with this for now

So plugins + themes + users should change all of their layout code to use this filter, and then change back at some point in the future?

😓

Member

pathawks commented Jun 15, 2017

considering that we want to get this in 3.5, let's go with this for now

So plugins + themes + users should change all of their layout code to use this filter, and then change back at some point in the future?

😓

@oe

This comment has been minimized.

Show comment
Hide comment
@oe

oe Jun 15, 2017

Member

you're right.... is this big enough of a usecase to warrant automatically doing it?

Member

oe commented Jun 15, 2017

you're right.... is this big enough of a usecase to warrant automatically doing it?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jun 15, 2017

Member

Well if they had this filter then we automatically applied it, then this filter would just no-op. Making it opt-in by just supplying the filter gives us space to get it right and make it fast before pushing it into the process for everyone. Thoughts?

Member

parkr commented Jun 15, 2017

Well if they had this filter then we automatically applied it, then this filter would just no-op. Making it opt-in by just supplying the filter gives us space to get it right and make it fast before pushing it into the process for everyone. Thoughts?

@pathawks

You're right. This filter is better than the current situation. We shouldn't let perfect be the enemy of good.

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Jun 15, 2017

Member

We shouldn't let perfect be the enemy of good.

👍 quote 👏 😃

Member

ashmaroli commented Jun 15, 2017

We shouldn't let perfect be the enemy of good.

👍 quote 👏 😃

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Jun 15, 2017

Member

@jekyllbot: merge +minor

Member

pathawks commented Jun 15, 2017

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 69e97fa into master Jun 15, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jun 15, 2017

Member

@jekyllbot: merge +minor

Member

parkr commented Jun 15, 2017

@jekyllbot: merge +minor

@jekyllbot jekyllbot deleted the strip-index-filter branch Jun 15, 2017

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jun 15, 2017

Member

LOL jinx.

Member

parkr commented Jun 15, 2017

LOL jinx.

jekyllbot added a commit that referenced this pull request Jun 15, 2017

@DirtyF DirtyF added this to the 3.5 milestone Jun 18, 2017

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