Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add URL checks to Doctor #5760

Merged
merged 3 commits into from Jul 18, 2017
Merged

Add URL checks to Doctor #5760

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c729d04
Add URL checks to Doctor
pathawks Jan 12, 2017
5195da5
Update formatting to match `healty?`
pathawks Feb 11, 2017
abacbaf
Use suggested warning messages
pathawks Jun 22, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 35 additions & 0 deletions lib/jekyll/commands/doctor.rb
View file
Open in desktop
@@ -1,3 +1,5 @@
require "addressable/uri"

module Jekyll
module Commands
class Doctor < Command
Expand Down Expand Up @@ -36,6 +38,7 @@ def healthy?(site)
!deprecated_relative_permalinks(site),
!conflicting_urls(site),
!urls_only_differ_by_case(site),
proper_site_url?(site),
].all?
end

Expand Down Expand Up @@ -91,6 +94,15 @@ def urls_only_differ_by_case(site)
urls_only_differ_by_case
end

def proper_site_url?(site)
url = site.config["url"]
[
url_exists?(url),
url_valid?(url),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DirtyF So, replace these two lines with:

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

?

Copy link
Member

@DirtyF DirtyF Jun 21, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

url_absolute(url),
].all?
end

private
def collect_urls(urls, things, destination)
things.each do |thing|
Expand All @@ -110,6 +122,29 @@ def case_insensitive_urls(things, destination)
(memo[dest.downcase] ||= []) << dest
end
end

def url_exists?(url)
return true unless url.nil? || url.empty?
Jekyll.logger.warn "Warning:", "You didn't set an URL in the config file, "\
"you may encounter problems with some plugins."
false
end

def url_valid?(url)
Addressable::URI.parse(url)
true
rescue
Jekyll.logger.warn "Warning:", "The site URL does not seem to be valid, "\
"check the value of `url` in your config file."
false
end

def url_absolute(url)
return true if Addressable::URI.parse(url).absolute?
Jekyll.logger.warn "Warning:", "Your site URL does not seem to be absolute, "\
"check the value of `url` in your config file."
false
end
end
end
end
Expand Down