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.

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 more log statements to site_github_munger. #189

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

NullableInt
Copy link

@NullableInt NullableInt commented Jul 2, 2020

This adds more logs that were critical for us to solve an deploy error.
Solves #186

This adds more logs that were critical for us to solve an deploy error.
Copy link
Contributor

@MichaelCurrin MichaelCurrin left a comment

Choose a reason for hiding this comment

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

Update log messages

lib/jekyll-github-metadata/site_github_munger.rb Outdated Show resolved Hide resolved
lib/jekyll-github-metadata/site_github_munger.rb Outdated Show resolved Hide resolved
NullableInt and others added 2 commits May 31, 2021 14:10
Updated with suggestions from @MichaelCurrin
Co-authored-by: Michael Currin <18750745+MichaelCurrin@users.noreply.github.com>
@@ -42,10 +42,13 @@ def drop

# Set `site.url` and `site.baseurl` if unset.
def add_url_and_baseurl_fallbacks!
site.config["url"] ||= Value.new("url", proc { |_c, r| r.url_without_path })
Jekyll::GitHubMetadata.log :debug, "Adding URL and base URL fallbacks..."
Copy link
Contributor

Choose a reason for hiding this comment

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

This describes the whole function, so some whitespace is useful, so indicate it applies to the section and not just the line or two directly after it.

Suggested change
Jekyll::GitHubMetadata.log :debug, "Adding URL and base URL fallbacks..."
Jekyll::GitHubMetadata.log :debug, "Adding URL and base URL fallbacks..."

return unless should_set_baseurl?

site.config["baseurl"] = Value.new("baseurl", proc { |_c, r| r.baseurl })
Jekyll::GitHubMetadata.log :debug, "baseurl is set to #{site.config["baseurl"]}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Use base URL here for consistency with the above.


Or use url and baseurl (lowercase, no spaces) throughout. With backticks or single quotes would be nice.

e.g.

"Adding `url` and `baseurl` fallbacks..."

and

"`baseurl` is set to #{site.config["baseurl"]}"

Copy link
Contributor

@MichaelCurrin MichaelCurrin left a comment

Choose a reason for hiding this comment

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

Apply suggestions consistently, thanks.

lib/jekyll-github-metadata/site_github_munger.rb Outdated Show resolved Hide resolved
Co-authored-by: Michael Currin <18750745+MichaelCurrin@users.noreply.github.com>
Copy link
Contributor

@MichaelCurrin MichaelCurrin left a comment

Choose a reason for hiding this comment

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

Thanks, changes look good

@@ -42,10 +42,14 @@ def drop

# Set `site.url` and `site.baseurl` if unset.
def add_url_and_baseurl_fallbacks!
site.config["url"] ||= Value.new("url", proc { |_c, r| r.url_without_path })
Jekyll::GitHubMetadata.log :debug, "Adding `url` and `baseurl` fallbacks..."
Copy link
Member

Choose a reason for hiding this comment

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

These would be better in the Value proc { } block, otherwise the benefits of using a Value (the fact that computation is deferred) is lost.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants