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

Rename & migrate representative_plugin as namespace plugin #1

Merged
merged 27 commits into from
Aug 14, 2023

Conversation

justinmayer
Copy link
Contributor

@justinmayer justinmayer commented Aug 8, 2023

khuevu and others added 25 commits May 6, 2014 00:27
clean_summary, read_more_link, representative_image, summary
plugins now use all_generators_finalized signal so as to avoid
issue #314. They fallback to current behavior is said signal is
not yet available.

See also: getpelican/pelican#1616

Closes #314.
* Enable Representative Image also for pages
* Add requirements.txt
* Add .vscode to .gitignore
@justinmayer
Copy link
Contributor Author

@schtobia replied to my comment about tests not passing for the Representative Image plugin, even in the monolithic repo, even before any rename/migration-related changes were made, and @schtobia said:

Hmm, it seems there is a problem I could use some help with.

It seems like the register() function is successfully connected on all_generators_finalized, but the registered function is never called, at least in the test environment.

Could you please give me some directions where to look further?

I have never used this plugin, and I'm away from my computer for the next few days. So I am ill-equipped to delve into this topic in detail at the moment.

@khuevu @jkrshw @Murchik @kernc @Lucas-C @pauloxnet: As folks who have contributed to this plugin in the past, might you have a moment to take a look and help @schtobia fix the failing tests? (cc: @avaris also, in case he has a moment)

@avaris
Copy link
Contributor

avaris commented Aug 13, 2023

I pushed a commit to fix the tests. The old version of tests would've never worked with the new signal. It connects the signals but the way tests are configured, it would never hit that signal, tests would need to actually run pelican. But that's not needed and changed the tests to only test the functionality.

However, pelican possibly needs a way to reliably get and set summary. Currently I don't think this plugin would be working "correctly". It's supposed to delete the images from summary but these modifications will not be propagated due to the caching of summary.

@justinmayer
Copy link
Contributor Author

justinmayer commented Aug 14, 2023

@avaris: Many thanks for fixing the tests. Let's create a new issue to track whether Pelican needs a way to get and set summary in a reliable manner.

Many thanks also to @khuevu for creating this plugin, to @schtobia for helping migrate it to its new home, and to all the folks who have contributed to the development of this plugin.

Featured Image: https://github.com/pelican-plugins/featured-image/releases/tag/1.0.0

@justinmayer justinmayer merged commit ccbd1a3 into main Aug 14, 2023
6 of 12 checks passed
@justinmayer justinmayer deleted the migrate branch August 14, 2023 08:41
@schtobia
Copy link
Collaborator

Thanks @avaris @justinmayer!

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.

8 participants