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 URL checks to Doctor #5760

Merged
merged 3 commits into from Jul 18, 2017

Conversation

Projects
None yet
4 participants
@pathawks
Member

pathawks commented Jan 12, 2017

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 doctor to make sure that things are in good working order.

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Jan 12, 2017

Member

I should probably add a check for baseurl as well, as that is also the source of much frustration.

Member

pathawks commented Jan 12, 2017

I should probably add a check for baseurl as well, as that is also the source of much frustration.

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks
Member

pathawks commented Jan 12, 2017

@parkr

parkr approved these changes Jan 20, 2017

LGTM! Not quite sure about the CI failures there, but 👍 on the content of the PR.

@jekyllbot jekyllbot removed the needs-work label Jan 20, 2017

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Feb 11, 2017

Member

Fixes #5718

Member

pathawks commented Feb 11, 2017

Fixes #5718

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Feb 11, 2017

Member

CI failures have been fixed 👍

Member

pathawks commented Feb 11, 2017

CI failures have been fixed 👍

@DirtyF

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.

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Feb 11, 2017

Member

So now you've got to set site.url?

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 jekyll doctor, not on every build. If a user is running jekyll doctor it's probably because something is not working as expected.

If a user is intentionally leaving site.url blank, they can safely disregard this warning.

Member

pathawks commented Feb 11, 2017

So now you've got to set site.url?

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 jekyll doctor, not on every build. If a user is running jekyll doctor it's probably because something is not working as expected.

If a user is intentionally leaving site.url blank, they can safely disregard this warning.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 31, 2017

Member

If a user is intentionally leaving site.url blank, they can safely disregard this warning.

Hm, maybe just don't run the check then?

Member

parkr commented Mar 31, 2017

If a user is intentionally leaving site.url blank, they can safely disregard this warning.

Hm, maybe just don't run the check then?

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Jun 21, 2017

Member

@pathawks so should we break those tests: one that checks if site.url exists and warn if not, and another that checks that when site.url exists, it's valid?

Member

DirtyF commented Jun 21, 2017

@pathawks so should we break those tests: one that checks if site.url exists and warn if not, and another that checks that when site.url exists, it's valid?

url = site.config["url"]
[
url_exists?(url),
url_valid?(url),

This comment has been minimized.

@pathawks

pathawks Jun 21, 2017

Member

@DirtyF So, replace these two lines with:

url_exists?(url) && url_valid?(url)

?

@pathawks

pathawks Jun 21, 2017

Member

@DirtyF So, replace these two lines with:

url_exists?(url) && url_valid?(url)

?

This comment has been minimized.

@DirtyF

DirtyF Jun 21, 2017

Member

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.

@DirtyF

DirtyF Jun 21, 2017

Member

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.

This comment has been minimized.

@pathawks

pathawks Jun 21, 2017

Member

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?

@pathawks

pathawks Jun 21, 2017

Member

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?

This comment has been minimized.

@DirtyF

DirtyF Jun 21, 2017

Member

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.
@DirtyF

DirtyF Jun 21, 2017

Member

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 added some commits Jan 12, 2017

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Jun 22, 2017

Member

Now with @DirtyF approved warning messages 👍

Member

pathawks commented on abacbaf Jun 22, 2017

Now with @DirtyF approved warning messages 👍

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Jun 22, 2017

Member

@pathawks BTW what do you think about adding a "check your configuration" section with jekyll doctor in the docs? I just realized this command is not documented on the website 😕

Member

DirtyF commented Jun 22, 2017

@pathawks BTW what do you think about adding a "check your configuration" section with jekyll doctor in the docs? I just realized this command is not documented on the website 😕

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Jun 22, 2017

Member

@DirtyF Good idea. We should also add a request to the PR template that users post the output of jekyll doctor

Member

pathawks commented Jun 22, 2017

@DirtyF Good idea. We should also add a request to the PR template that users post the output of jekyll doctor

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Jun 24, 2017

Member

For an example of what I hope to accomplish with this: jekyll/jekyll-sitemap#171

Member

pathawks commented Jun 24, 2017

For an example of what I hope to accomplish with this: jekyll/jekyll-sitemap#171

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Jun 24, 2017

Member

We should also add a request to the PR template that users post the output of jekyll doctor

Thank you @DirtyF for #6169

(Breadcrumbs 🥖)

Member

pathawks commented Jun 24, 2017

We should also add a request to the PR template that users post the output of jekyll doctor

Thank you @DirtyF for #6169

(Breadcrumbs 🥖)

@DirtyF

DirtyF approved these changes Jul 18, 2017

@DirtyF DirtyF added the enhancement label Jul 18, 2017

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Jul 18, 2017

Member

@jekyllbot: merge +minor

Thanks @pathawks ❤️

Member

DirtyF commented Jul 18, 2017

@jekyllbot: merge +minor

Thanks @pathawks ❤️

@jekyllbot jekyllbot merged commit da65e94 into master Jul 18, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jekyllbot jekyllbot deleted the pr/url-doctor branch Jul 18, 2017

jekyllbot added a commit that referenced this pull request Jul 18, 2017

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