Add URL checks to Doctor#5760
Conversation
| !conflicting_urls(site), | ||
| !urls_only_differ_by_case(site) | ||
| !urls_only_differ_by_case(site), | ||
| proper_site_url?(site) |
There was a problem hiding this comment.
If only Rubocop allowed Style/TrailingCommaInLiteral 😡
There was a problem hiding this comment.
@pathawks what prevents us from adding Style/TrailingCommaInLiteral: { EnforcedStyleForMultiline: consistent_comma }to .rubocop.yml?
|
I should probably add a check for |
|
/cc: @jekyll/core @jekyll/ecosystem |
parkr
left a comment
parkr
left a comment
There was a problem hiding this comment.
LGTM! Not quite sure about the CI failures there, but 👍 on the content of the PR.
pathawks
force-pushed
the
pr/url-doctor
branch
from
fd13545 to
c123b10
Compare
|
Fixes #5718 |
|
CI failures have been fixed 👍 |
DirtyF
left a comment
DirtyF
left a comment
There was a problem hiding this comment.
So now you've got to set site.url? Before it was not mandatory and was automatically set in development mode or by GitHub Pages.
It's not mandatory, but since leaving it blank can cause problems with plugins or themes, users could at least be made aware of the situation. Note that this only runs when a user runs If a user is intentionally leaving |
Hm, maybe just don't run the check then? |
|
@pathawks so should we break those tests: one that checks if |
| url = site.config["url"] | ||
| [ | ||
| url_exists?(url), | ||
| url_valid?(url), |
There was a problem hiding this comment.
There was a problem hiding this comment.
I was thinking of moving url_exists? out of proper_site_url? in a separate single test to address Parker's concern, so we test first if site.url exists before launching proper_site_url. But as I suck at logic and programming, this is maybe a terrible idea :)
If what you are proposing does that, it's fine.
There was a problem hiding this comment.
I don't follow.
We really want site.url to be set, either manually by the user or automatically by something like pages-gem. If site.url is blank, it is likely to cause problems and we should warn the user about this.
If a user has some special reason to use a blank site.url then they will understand that this warning does not apply to them.
On the other hand, if a user has simply neglected to set site.url and now things aren't working as expected, this check should alert them to the issue and suggest setting site.url. That is the entire point of this PR.
I'm probably not understanding your concerns; can you try to explain to me again now that I've explained my intent?
There was a problem hiding this comment.
I get you perfectly and as it's impossible to tell if a blank site.url is intentional or not, you prefer to warn in both cases.
Maybe just make the messages more clear then?
bundle exec jekyll doctor
Warning: You didn't set an URL in the config file, you may encounter problems with some plugins.bundle exec jekyll doctor
Warning: The site URL does not seem to be valid, check the value of `url` in your config file.bundle exec jekyll doctor
Warning: Your site URL does not seem to be absolute, check the value of `url` in your config file.
|
@pathawks BTW what do you think about adding a "check your configuration" section with |
|
@DirtyF Good idea. We should also add a request to the PR template that users post the output of |
|
For an example of what I hope to accomplish with this: jekyll/jekyll-sitemap#171 |
|
@jekyllbot: merge +minor Thanks @pathawks ❤️ |
4 participants
If a site does not provide an absolute URL in
site.url, all sorts of wonky behavior can creep up, especially as sites rely more heavily on themes plugins that assume standard behavior.To this end, I would like to add some simple URL checks to
jekyll doctorto make sure that things are in good working order.