-
Notifications
You must be signed in to change notification settings - Fork 113
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
Include absolute path in canonical url #109
Conversation
Use url and baseurl to generate prefix for redirect url, but prefer github specific configuration if set. This leads to absolute paths as suggested. Fixes jekyll#29
@@ -69,7 +69,7 @@ def redirect_url(site, item) | |||
end | |||
|
|||
def redirect_prefix(site) | |||
config_github_url(site) || config_baseurl(site) || "" | |||
config_github_url(site) || "#{config_url(site)}#{config_baseurl(site)}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can / should URI.join
here.
@benbalter there you go |
def config_url(site) | ||
url = site.config.fetch('url', nil) || "" | ||
baseurl = site.config.fetch('baseurl', nil) || "" | ||
File.join url, baseurl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be URI.join
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
URI.join
seems to fail for relative urls. Thus, I went with File.join
like a few lines above.
See discussion of my first commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could both of the fetches be rewritten as fetch <KEY>, ""
rather than fetch <KEY>, nil || ""
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That breaks the tests. If the key is explicitly set to null, it returns null / not the default value, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. Makes sense. Good catch.
🆒 This LGTM. @jekyll/plugin-core, please 👀 |
@parkr is @jekyll/plugin-core a real thing? Leads to an http 404... |
@jekyllbot: merge |
Use url and baseurl to generate prefix for redirect url, but prefer github
specific configuration if set. This leads to absolute paths as suggested.
Fixes #29