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

Excerpt relative-path should match its path #6597

Merged
merged 3 commits into from Feb 20, 2018

Conversation

Projects
None yet
6 participants
@ashmaroli
Member

ashmaroli commented Dec 4, 2017

Currently a document's excerpt's relative path equals the doc's relative path instead of the excerpt-path relative to the site's source.

Before

             doc.path: my_blog/_posts/2017-12-04-some-doc.md
         excerpt.path: my_blog/_posts/2017-12-04-some-doc.md/#excerpt
    doc.relative_path: _posts/2017-12-04-some-doc.md
excerpt.relative_path: _posts/2017-12-04-some-doc.md

After

             doc.path: my_blog/_posts/2017-12-04-some-doc.md
         excerpt.path: my_blog/_posts/2017-12-04-some-doc.md/#excerpt
    doc.relative_path: _posts/2017-12-04-some-doc.md
excerpt.relative_path: _posts/2017-12-04-some-doc.md/#excerpt

@ashmaroli ashmaroli added this to the v3.7.0 milestone Dec 4, 2017

@oe

oe approved these changes Dec 4, 2017

@mattr-

The tests need fixing, at a minimum

@mattr-

This comment has been minimized.

Member

mattr- commented Dec 4, 2017

Can we do this for 3.7.0 since the excerpt's file path is made available to liquid? It would depend on whether or not we'd consider anything we export to liquid as part of a public facing API that we would need to keep backwards compatible. I tend to think that it is, but I'll let others weigh in.

@ashmaroli

This comment has been minimized.

Member

ashmaroli commented Dec 4, 2017

@mattr- I didn't think this would've been a breaking change till I saw the Travis failures.. This cannot be shipped until 4.0 since the tests clearly document the current expected behavior even on test_document.rb

@ashmaroli ashmaroli modified the milestones: v3.7.0, 4.0 Dec 4, 2017

@ashmaroli

This comment has been minimized.

Member

ashmaroli commented Dec 4, 2017

related:

def_delegator :@obj, :relative_path, :path

@DirtyF

This comment has been minimized.

Member

DirtyF commented Dec 4, 2017

I didn't think this would've been a breaking change

What would it break for our users?

@ashmaroli

This comment has been minimized.

Member

ashmaroli commented Dec 6, 2017

Designated as a 4.0 change because this alters the outcome of an existing public_method :relative_path

@DirtyF

Let's just fix the broken test and get this in 3.7.0, it's just a log output.

@ashmaroli

This comment has been minimized.

Member

ashmaroli commented Dec 15, 2017

it's just a log output

@DirtyF Please check #6621

@DirtyF

This comment has been minimized.

Member

DirtyF commented Dec 15, 2017

I have (hence my above comment)

@parkr

This needs a test.

As for the breaking change argument, this is a bug fix. We often change the return value of methods in updates to Jekyll (this is part of normal development). What we're trying to avoid is changing method names altogether, which break other programs like plugins. We can absolutely change the output in any release we choose.

ashmaroli added some commits Feb 4, 2018

@DirtyF

DirtyF approved these changes Feb 4, 2018

@oe oe modified the milestones: 4.0, v3.8.0 Feb 11, 2018

@oe oe referenced this pull request Feb 17, 2018

Closed

Release 3.8.0 #6783

12 of 13 tasks complete
@oe

oe approved these changes Feb 20, 2018

LGTM!

@parkr

parkr approved these changes Feb 20, 2018

@DirtyF

This comment has been minimized.

Member

DirtyF commented Feb 20, 2018

@jekyllbot: :shipit: +minor

@jekyllbot jekyllbot merged commit 5ebdc18 into jekyll:master Feb 20, 2018

3 checks passed

WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment