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

Choose logging handler via --log-handler CLI option #3293

Merged
merged 6 commits into from
May 30, 2024

Conversation

cpitclaudel
Copy link
Contributor

@cpitclaudel cpitclaudel commented Mar 12, 2024

  • pelican/__init__.py (parse_arguments): Declare new --log-handler argument.
    Uses a string instead of a type= argument to get better error messages when passed an incorrect choice.
    (main): Pass args.logs_handler to init_logging
  • pelican/log.py: Expose default log handler as DEFAULT_LOG_HANDLER.
    (init): Pass handler to logging.basicConfig.

Pull Request Checklist

Resolves: GH-3292

  • Ensured tests pass and (if applicable) updated functional test output
  • Conformed to code style guidelines by running appropriate linting tools
  • Added tests for changed code
    Question: I think there's no tests for the logger?
  • Updated documentation for changed code
    Question: I think there's no separate documentation for the CLI, I just added an argparse help

* pelican/__init__.py (parse_arguments): Declare new --logs-handler argument.
Uses a string instead of a `type=` argument to get better error messages when
passed an incorrect choice.
(main): Pass `args.logs_handler` to `init_logging`
* pelican/log.py: Expose default log handler as `DEFAULT_LOG_HANDLER`.
(init): Pass handler to `logging.basicConfig`.

Closes getpelicanGH-3292.
pelican/__init__.py Outdated Show resolved Hide resolved
@justinmayer justinmayer changed the title Add --logs-handler CLI option Choose logging handler via --logs-handler CLI option Mar 12, 2024
@cpitclaudel cpitclaudel requested a review from justinmayer March 23, 2024 13:22
Copy link
Member

@justinmayer justinmayer left a comment

Choose a reason for hiding this comment

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

Hi @cpitclaudel. Thank you for the quick corrections. Apologies for the delay in moving this forward. Upon another look, I think I would prefer to standardize on "log" instead of the plural "logs". We already have DEFAULT_LOG_HANDLER, for example, and therefore I think it would be more consistent to use LOG_HANDLERS, --log-handler, et cetera, instead of the plural versions. Unless there are any objections, would you be so kind as to make those changes so we can merge your contribution?

@cpitclaudel
Copy link
Contributor Author

No worries! No objection from me (pushed), but note that this is inconsistent with --logs-dedup-min-level.

@justinmayer justinmayer changed the title Choose logging handler via --logs-handler CLI option Choose logging handler via --log-handler CLI option Apr 16, 2024
Copy link
Member

@justinmayer justinmayer left a comment

Choose a reason for hiding this comment

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

I saw that but decided that particularly discrepancy is okay for now. We shall strive for eventual consistency! 😉

@avaris: Any thoughts on this PR?

@justinmayer justinmayer requested review from offbyone and avaris April 16, 2024 15:50
@offbyone
Copy link
Contributor

I'm curious what use this flag would be put to. It would be constructive, I think, to include documentation for how to use it, how to define a new handler, and an example custom handler.

@cpitclaudel
Copy link
Contributor Author

I'm curious what use this flag would be put to.

I use it in makefiles, because the forced line wrapping that the rich handler does breaks hyperlinks:

           ERROR    Skipping                                     contents.py:175
                    /home/xxx/xxx/xxxxxxx/xxxxx/xxxxx/xxxxxxxxxx                
                    /xxxxxxxxxxxxx/xxxxxxxx/xxxxxxxxx/xxxxxx.htm                
                    l: could not find information about 'date'           

With this format my terminal and my IDE don't let me click on the file in question to navigate to it.

It would be constructive, I think, to include documentation for how to use it, how to define a new handler, and an example custom handler.

Defining a custom handler is something the Pelican devs would do, not users. These are standard Python log handlers, so the appropriate documentation would be https://docs.python.org/3/library/logging.handlers.html

@GiovanH
Copy link
Contributor

GiovanH commented May 19, 2024

I was just about to request this. Are there any outstanding issues or is this fully approved?

@justinmayer
Copy link
Member

Unless there are further comments from others, I intend to merge the Ruff-related PR (3239) and then this one.

@justinmayer
Copy link
Member

Many thanks for your enhancement, Clément! 🌟

@justinmayer justinmayer merged commit 39c9644 into getpelican:master May 30, 2024
15 checks passed
@egberts
Copy link
Contributor

egberts commented Jul 5, 2024

WOW! Now if we can re-add the name back into LOG_FORMAT for log-handler=plain option, then the --log-handler=plain output would look like the following:

$ python -m pelican --log-handler=plain -s /tmp/pelicanconf-invalid-PATH-1
load_source: AttributeError: 'NoneType' object has no attribute 'loader'

See the word load_source (that is in settings.py source file, but we now know what to look for). That is a nice thing to have.

Instead of the currently HEAD commit shows the following:

$ python -m pelican --log-handler=plain -s /tmp/pelicanconf-invalid-PATH-1
AttributeError: 'NoneType' object has no attribute 'loader'

Currently, we have no idea which method/procedures (that %(name)s in LOG_FORMAT can provide for) when such an log output occurs (not to mention the missing <source>:<line-no> column.

Furthermore, I've investigated a quick fix for the above and was unable to leverage the above but applied patch which rendered the inability to use such %(name)s format (and possibly ignores LOG_FORMAT).

@cpitclaudel
Copy link
Contributor Author

@egberts I'm sorry, I don't really understand what you're saying

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.

Option to disable the RichHandler log handler?
5 participants