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 debug logging to assist in troubleshooting #144

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kriation
Copy link

@kriation kriation commented Nov 16, 2019

Recently, I was struggling to understand why archives was generating duplicate pages. Without debug logging in how archives was processing each set of pages, it was difficult to determine where the issue was.

I added the code in this PR to assist and hope to close issue #145

@ashmaroli
Copy link
Member

@kriation Instead of leaving the opening comment of a Pull Request empty and opening a related issue-ticket, it is better if the PR's raison d'être is contained within the PR itself.

However, if you're submitting a PR to resolve an existing issue ticket, you can include
Closes <Issue No.> in the opening component.

@ashmaroli
Copy link
Member

I don't have a sample site at hand to test this manually. Can you please post screenshots of the debug output from this change?
Thanks.

@kriation
Copy link
Author

Can you please post screenshots of the debug output from this change? - @ashmaroli

Attached! 2019-11-18_14:35:08

@DirtyF
Copy link
Member

DirtyF commented Nov 19, 2019

If this is is accepted, this should be documented on the README.

@ashmaroli
Copy link
Member

There are couple of things immediately apparent from the screenshot:

  • This adds additional noise to an already noisy verbose output. 🙄 Imagine what the output would be for a site with hundreds of posts..
  • categories should be debugged as with category: #{title} not tag:
  • Jekyll.logger.debug has been called with just a single parameter instead of two (missing comma between the two strings..)

All in all, I'm not in favor of this proposal. Sorry.

@DirtyF
Copy link
Member

DirtyF commented Nov 19, 2019

@ashmaroli IIUC this will only be output when you use --debug flag, not by default right?

@ashmaroli
Copy link
Member

ashmaroli commented Nov 19, 2019

will only be output when you use --debug flag

Yes, that is correct. I was thinking maybe instead of piping into the default debug output, it'd be better if these output was triggered via an additional configuration... For example,

jekyll-archives:
  debug: true

But that needs to be approved by other maintainers first.

@kriation
Copy link
Author

@ashmaroli - I'm happy to make the changes you mentioned.

With regard to noisy verbose output, isn't verbosity verbose by design?

Without this logic, resolving issues (as I did) would take considerably longer as there are no other mechanisms to debug what is being processed in this plugin.

@ashmaroli
Copy link
Member

noisy verbose output, isn't verbosity verbose by design? ...
...no other mechanisms to debug what is being processed

Unfortunately, true, to both questions.
Perhaps you can use a private helper to iterate through the list of posts and log each post to the debug output..

resolving issues (as I did)

What exactly was that issue?

@kriation
Copy link
Author

kriation commented Nov 19, 2019

Perhaps you can use a private helper to iterate through the list of posts and log each post to the debug output..

How is this different than using the debug logger?

What exactly was that issue?

Nuance in how I specified tags in post front matter.

@ashmaroli
Copy link
Member

How is this different than using the debug logger?

Greater control and flexibility.. For example:

def read_tags
  if enabled? "tags"
    tags.each do |title, posts|
      debug_key(posts, title, "tag")
      @archives << Archive.new(@site, title, "tag", posts)
    end
  end
end

private

def debug_key(posts, title, key)
  posts.each do |post|
    debug "Processing #{post.relative_path} with #{key}: #{title}"
  end
end

def debug_date(...)
  ...
end

def debug(msg)
  return unless site.config.dig("jekyll_archives", "debug")
  Jekyll.logger.debug "Archives:", msg
end

Disclaimer: The above example needs to be optimized further..

@kriation
Copy link
Author

Understood! I'll update my branch with the changes.

@ashmaroli
Copy link
Member

I'll update my branch with the changes.

I was hoping for some inputs or approval of that idea from one of the other maintainers before you made additional changes..
Either ways, the config key for this plugin is jekyll-archives instead of jekyll_archives in my example.
ref: https://github.com/jekyll/jekyll-archives/blob/master/docs/configuration.md

lib/jekyll-archives.rb Outdated Show resolved Hide resolved
Co-Authored-By: Frank Taillandier <[email protected]>
@ashmaroli ashmaroli requested a review from a team January 27, 2020 16:33
Copy link
Member

@DirtyF DirtyF left a comment

Choose a reason for hiding this comment

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

The debug output should only be displayed when using the --debug option

@ashmaroli
Copy link
Member

The debug output should only be displayed when using the --debug option

@DirtyF That's how it is already. Jekyll.logger.debug cannot output in default level (:info)

@ashmaroli
Copy link
Member

@DirtyF What is your opinion on resolving RuboCop offenses on this branch by using the example in #144 (comment) as a guide..?

@DirtyF
Copy link
Member

DirtyF commented Jan 29, 2020

fine by me

@ashmaroli
Copy link
Member

@kriation Thank you for bring this PR up to date. But my requests for changes still remains.. I'll approve if you're able to refactor this to meet the following:

  • The proposed debug output is an opt-in — users have to explicitly enable {"jekyll-archives"=>{ "debug"=>true}} via their config file.
  • Have the above documented in the docs/configuration.md file.
  • Extract the debugging logic into a private method that prints each archive entry in a new line. For example:
          Archives: Processing documents with the tag: 'foobar'
                    _posts/2018-05-06-hello-world.markdown
                    _posts/2018-05-08-hello-again.markdown
                    _posts/2018-05-12-the-truth-is-out-there.markdown
                    _posts/2019-06-07-hold-the-door.markdown
                    _posts/2019-07-14-truth-justice-and-stuff.markdown
                    _posts/2020-07-04-static-is-the-new-black.markdown
          Archives: Processing documents with the category: 'lipsum'
                    _posts/2018-05-06-hello-world.markdown
                    _posts/2018-05-08-hello-again.markdown
                    _posts/2018-05-12-the-truth-is-out-there.markdown
                    _posts/2019-06-07-hold-the-door.markdown
                    _posts/2019-07-14-truth-justice-and-stuff.markdown
                    _posts/2020-07-04-static-is-the-new-black.markdown
          Archives: Processing documents in the year: '2019'
                    _posts/2019-06-07-hold-the-door.markdown
                    _posts/2019-07-14-truth-justice-and-stuff.markdown
          Archives: Processing documents in the month: '2018/05'
                    _posts/2018-05-06-hello-world.markdown
                    _posts/2018-05-08-hello-again.markdown
                    _posts/2018-05-12-the-truth-is-out-there.markdown
          Archives: Processing documents from the day: '2018/05/06'
                    _posts/2018-05-06-hello-world.markdown

@kriation
Copy link
Author

kriation commented Jul 7, 2020

@ashmaroli Absolutely! I noticed earlier this week that the original commits were well behind the existing mainline, so this last commit was to bring it up to par. I'll take a stab at implementing your changes and will add it to the commit list for this PR.

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.

3 participants