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

Improve logging of generators and writer loaders #2821

Merged

Conversation

lgiordani
Copy link
Contributor

This improves the logging of get_generator_classes() to provide the source of the generator (either internal or from a plugin). It also reviews the code to make it clearer (in my opinion, but I'm biased 😉), renames get_writer() to _get_writer_class() for consistency with the renamed _get_generator_classes(). Both are purely internal methods so I think it makes sense to signal it with the initial underscore.

Aside from cosmetic changes this doesn't affect the Pelican loading mechanism at all.

This is my first PR for this project, so please help me double checking that everything was done according to the guidelines.

Pull Request Checklist

  • Ensured tests pass and (if applicable) updated functional test output
  • Conformed to code style guidelines by running appropriate linting tools

This improves the logging of get_generator_classes() to
provide the source of the generator (either internal or from a plugin).
It also reviews the code to make it clearer (in my opinion, but I'm biased ;)), renames get_writer() to _get_writer_class() for consistency with the renamed _get_generator_classes().
@lgiordani
Copy link
Contributor Author

I didn't realise tests were running on my forked branch as well 🤦‍♂️ Good, first mistake done. I'll fix that and come back, sorry!

@avaris
Copy link
Member

avaris commented Nov 21, 2020

I'll look in more detail later but wanted to mention the logging scheme we use: Use %s placeholders instead and pass values as arguments. That allows processing of arguments when necessary as well as ability to filter them on the user side.

@lgiordani
Copy link
Contributor Author

Sorry, I should have noticed that part on the contribution document you mentioned. I will change the PR accordingly

pelican/__init__.py Outdated Show resolved Hide resolved
pelican/__init__.py Outdated Show resolved Hide resolved
pelican/__init__.py Outdated Show resolved Hide resolved
Copy link
Member

@avaris avaris left a comment

Choose a reason for hiding this comment

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

Looks good, thanks 👍

@justinmayer
Copy link
Member

Many thanks to @lgiordani for the enhancements and to @avaris for reviewing. ✨

@justinmayer justinmayer merged commit afdf0fb into getpelican:master Nov 22, 2020
@justinmayer
Copy link
Member

Thanks to AutoPub, these changes have been released as Pelican v4.5.2. 🎉

@lgiordani
Copy link
Contributor Author

Thanks both! I learned more than I expected 🎉

@lgiordani lgiordani deleted the review-generators-and-writer-loaders branch November 22, 2020 16:41
Comment on lines +181 to +183
discovered_generators.extend(
[(generator, receiver.__module__) for generator in values]
)
Copy link

Choose a reason for hiding this comment

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

Is this meant to be inside of the if not isinstance(...) check? If values is already an iterable from get_generators.send() call then this code will never run (and they won't be added to discovered_generators).

Copy link
Member

Choose a reason for hiding this comment

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

Oh, you are right. I missed the indent level in the diff 🤦 .

Copy link

Choose a reason for hiding this comment

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

That was my guess! 😢 I think the old behavior just needs to de-dent these three lines!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, snap, sorry guys, such a stupid mistake. I'll send a fix asap

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #2823 Thanks @clokep for pointing it out

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request May 14, 2023
Pkgsrc changes :
* updated dependencies : removed py-six and added py-rich

Upstream changes :

- Pelican 4.5.1 :
  * Refactor intra-site link discovery in order to match more permissively
  * Fix plugins running twice in auto-reload mode
  * Add notice to use from pelican import signals instead of
    import pelican.signals
- Pelican 4.5.2 : Improve logging of generators and writer loaders
- Pelican 4.5.3 : Fix a mistake made in PR #2821
  (getpelican/pelican#2821)
- Pelican 4.5.4 : Replace plugin definitions in settings with string
  representations after registering, so they can be cached correctly.
- Pelican 4.6.0 :
  * Add new URL pattern to PAGINATION_PATTERNS for the last page in the list
  * Speed up livereload Invoke task via caching
  * Ignore None return value from get_generators signal
  * Relax dependency minimum versions and remove upper bounds
- Pelican 4.7.0 :
  * Improve default theme rendering on mobile and other small screen devices
  * Add support for hidden articles
  * Improve word count behavior when generating summary CJK & other locales
  * Add progress spinner during generation and richer logging, both via Rich
  * Invoke tasks serve and livereload now auto-open a web browser pointing to
    the locally-served web site
  * Support some date format codes used by ISO dates
  * Document how to add a new writer
- Pelican 4.7.1 :
  * Extend rich logging to server component
  * Fix an issue where metadata flagged to be discarded was being cached
  * Adjust suffix in server to allow redirection when needed
  * Add MIME types for web fonts
  * Distribute sample data used to run tests
  * Add Python 3.10 to test matrix
- Pelican 4.7.2 :
  * Fix incorrect parsing of parameters specified via -e / --extra-settings
    option flags
  * Add categories.html template to default theme
  * Document how to use plugins to inject content
- Pelican 4.8.0 :
  * Use JSON values for extra settings in Invoke tasks template
  * Add content tag for links, which can help with things like Twitter
    social cards
  * Improve word count behavior when generating summary
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.

4 participants