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

Backwards-compatibilize relative permalinks #1081

Merged
merged 12 commits into from May 12, 2013

Conversation

Projects
None yet
4 participants
@parkr
Member

parkr commented May 11, 2013

Fix for #1077

This is both

  • deprecation message on build/serve
  • jekyll doctor command

// @mojombo @benbalter @zachgersh

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter May 11, 2013

Contributor

Maybe I'm misreading the diff, but where does it handle absolute permalinks if relative_permalinks is false?

Contributor

benbalter commented May 11, 2013

Maybe I'm misreading the diff, but where does it handle absolute permalinks if relative_permalinks is false?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 11, 2013

Member

I haven't modified the write behaviour yet - working on that.

Member

parkr commented May 11, 2013

I haven't modified the write behaviour yet - working on that.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 11, 2013

Member

Output for build:

~/code/jekyll/tsite$ be ../bin/jekyll build
Configuration file: /Users/parker/code/jekyll/tsite/_config.yml
            Source: /Users/parker/code/jekyll/tsite
       Destination: /Users/parker/code/jekyll/tsite/_site
      Generating...
       Deprecation: Starting in 1.1, permalinks for pages in subfolders must be absolute permalinks relative to the site source.
                    ...done.

Output for doctor:

~/code/jekyll/tsite$ be ../bin/jekyll doctor
Configuration file: /Users/parker/code/jekyll/tsite/_config.yml
       Deprecation: 'test/foo.md' uses relative permalinks which will be automatically deprecated in Jekyll v1.1.
       Deprecation: 'test/index.md' uses relative permalinks which will be automatically deprecated in Jekyll v1.1.
Member

parkr commented May 11, 2013

Output for build:

~/code/jekyll/tsite$ be ../bin/jekyll build
Configuration file: /Users/parker/code/jekyll/tsite/_config.yml
            Source: /Users/parker/code/jekyll/tsite
       Destination: /Users/parker/code/jekyll/tsite/_site
      Generating...
       Deprecation: Starting in 1.1, permalinks for pages in subfolders must be absolute permalinks relative to the site source.
                    ...done.

Output for doctor:

~/code/jekyll/tsite$ be ../bin/jekyll doctor
Configuration file: /Users/parker/code/jekyll/tsite/_config.yml
       Deprecation: 'test/foo.md' uses relative permalinks which will be automatically deprecated in Jekyll v1.1.
       Deprecation: 'test/index.md' uses relative permalinks which will be automatically deprecated in Jekyll v1.1.
Show outdated Hide outdated lib/jekyll/site.rb
Show outdated Hide outdated lib/jekyll/page.rb
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 11, 2013

Member

Should definitely loop in @mattr-.

Member

parkr commented May 11, 2013

Should definitely loop in @mattr-.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 11, 2013

Member

Error is a git error on Travis's side. @benbalter Ready for final scrutiny.

Member

parkr commented May 11, 2013

Error is a git error on Travis's side. @benbalter Ready for final scrutiny.

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter May 11, 2013

Contributor

Mobile, but couldn't tell from above. Does it err out on root pages with
explicit permalinks?
On May 11, 2013 1:54 PM, "Parker Moore" notifications@github.com wrote:

Error is a git error on Travis's side. @benbalterhttps://github.com/benbalterReady for final scrutiny.


Reply to this email directly or view it on GitHubhttps://github.com/jekyll/jekyll/pull/1081#issuecomment-17764316
.

Contributor

benbalter commented May 11, 2013

Mobile, but couldn't tell from above. Does it err out on root pages with
explicit permalinks?
On May 11, 2013 1:54 PM, "Parker Moore" notifications@github.com wrote:

Error is a git error on Travis's side. @benbalterhttps://github.com/benbalterReady for final scrutiny.


Reply to this email directly or view it on GitHubhttps://github.com/jekyll/jekyll/pull/1081#issuecomment-17764316
.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 11, 2013

Member

@benbalter It detects pages in the root with explicit permalinks, yeah. Once you're back, my comment above shows the output of jekyll doctor, which captures this case.

Member

parkr commented May 11, 2013

@benbalter It detects pages in the root with explicit permalinks, yeah. Once you're back, my comment above shows the output of jekyll doctor, which captures this case.

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter May 11, 2013

Contributor

But those pages are not affected, correct?
On May 11, 2013 2:15 PM, "Parker Moore" notifications@github.com wrote:

@benbalter https://github.com/benbalter It detects pages in the root
with explicit permalinks, yeah. Once you're back, my comment above shows
the output of jekyll doctor, which captures this case.


Reply to this email directly or view it on GitHubhttps://github.com/jekyll/jekyll/pull/1081#issuecomment-17764650
.

Contributor

benbalter commented May 11, 2013

But those pages are not affected, correct?
On May 11, 2013 2:15 PM, "Parker Moore" notifications@github.com wrote:

@benbalter https://github.com/benbalter It detects pages in the root
with explicit permalinks, yeah. Once you're back, my comment above shows
the output of jekyll doctor, which captures this case.


Reply to this email directly or view it on GitHubhttps://github.com/jekyll/jekyll/pull/1081#issuecomment-17764650
.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 11, 2013

Member

Those pages are effected insofar as they use "/" as their base.

Member

parkr commented May 11, 2013

Those pages are effected insofar as they use "/" as their base.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 11, 2013

Member

So they are considered as pages which use relative links, but the relative link is just "/" so it's fine :)

Member

parkr commented May 11, 2013

So they are considered as pages which use relative links, but the relative link is just "/" so it's fine :)

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter May 11, 2013

Contributor

Re: Above, if my link isn't going to break, you shouldn't tell me it's going to break. Doesn't the leading / explicitly make it absolute?

After testing, a few things:

  • Need success message for doctor if no errors, otherwise I think it failed or something
  • "Automatically" deprecated? How about just deprectated. A bit redundant
  • Root pages are not going to be deprecated... no need to warn. See above
  • Any permalink in any subfolder when relative_permalinks: true should warn.*
  • Doctor should be cool with permalinks in subfolders if config says relative_permalinks: false

* If I have a folder called foo and the permalink of page bar.md is foo/bar, I should still yell at me. My explicit intent (pre 1.0) is foo/foo/bar, which will break with absolute permalinks. No need to look at the parent folder name, ever AFAIK.

Logic should be:

  1. Did they explicitly tell Jekyll to use absolute permalinks? Great. They're good to go. They know their 💩
  2. Loop through pages... for any page:
    1. Is it in a sub folder?
    2. Did they specify a permalink?

If both sub points of 2. are true, yell at me. If only one is true, they're fine. We're not going to deprecate them.

Will double check the page creation, but otherwise, I'd say 👍 with some of the above feedback refinements.

Contributor

benbalter commented May 11, 2013

Re: Above, if my link isn't going to break, you shouldn't tell me it's going to break. Doesn't the leading / explicitly make it absolute?

After testing, a few things:

  • Need success message for doctor if no errors, otherwise I think it failed or something
  • "Automatically" deprecated? How about just deprectated. A bit redundant
  • Root pages are not going to be deprecated... no need to warn. See above
  • Any permalink in any subfolder when relative_permalinks: true should warn.*
  • Doctor should be cool with permalinks in subfolders if config says relative_permalinks: false

* If I have a folder called foo and the permalink of page bar.md is foo/bar, I should still yell at me. My explicit intent (pre 1.0) is foo/foo/bar, which will break with absolute permalinks. No need to look at the parent folder name, ever AFAIK.

Logic should be:

  1. Did they explicitly tell Jekyll to use absolute permalinks? Great. They're good to go. They know their 💩
  2. Loop through pages... for any page:
    1. Is it in a sub folder?
    2. Did they specify a permalink?

If both sub points of 2. are true, yell at me. If only one is true, they're fine. We're not going to deprecate them.

Will double check the page creation, but otherwise, I'd say 👍 with some of the above feedback refinements.

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter May 11, 2013

Contributor

👍

Contributor

benbalter commented May 11, 2013

👍

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 11, 2013

Member

Just want @mattr- to look this over and we'll be in business :)

Member

parkr commented May 11, 2013

Just want @mattr- to look this over and we'll be in business :)

@benbalter benbalter referenced this pull request May 11, 2013

Closed

Document new features #1072

4 of 4 tasks complete
site.read
unless deprecated_relative_permalinks(site)
Jekyll::Logger.info "Your test results", "are in. Everything looks fine."

This comment has been minimized.

@mattr-

mattr- May 12, 2013

Member

I'm not intimately familiar with Jekyll::Logger. Why is there a comma here? What am I not aware of that keeps it from being just one string?

@mattr-

mattr- May 12, 2013

Member

I'm not intimately familiar with Jekyll::Logger. Why is there a comma here? What am I not aware of that keeps it from being just one string?

This comment has been minimized.

@parkr

parkr May 12, 2013

Member

Jekyll::Logger's methods take a topic and a message. The topic is rjust(20)'d then the message is printed. This splits up the message so that it's not [20 spaces]Your test results are in....

@parkr

parkr May 12, 2013

Member

Jekyll::Logger's methods take a topic and a message. The topic is rjust(20)'d then the message is printed. This splits up the message so that it's not [20 spaces]Your test results are in....

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- May 12, 2013

Member

👍 from me. I don't think my question is a giant blocker.

Member

mattr- commented May 12, 2013

👍 from me. I don't think my question is a giant blocker.

parkr added a commit that referenced this pull request May 12, 2013

Merge pull request #1081 from mojombo/relative-permalinks
Backwards-compatibilize relative permalinks

@parkr parkr merged commit 28b9e35 into master May 12, 2013

1 check passed

default The Travis CI build passed
Details

@parkr parkr deleted the relative-permalinks branch May 12, 2013

parkr added a commit that referenced this pull request May 12, 2013

parkr added a commit that referenced this pull request May 12, 2013

parkr added a commit that referenced this pull request May 12, 2013

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

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