pelican-plugins / sitemap

Generates a site map for Pelican-powered sites
50 stars 11 forks source link

Re-factor Sitemap plugin #3

Closed kernc closed 1 year ago

kernc commented 4 years ago

This is an attempt at PoC simplification (ymmv) of the plugin. Comments and ideas welcome!

kernc commented 4 years ago

I'd make this a class, keep state that way and write everything at the end.

Thanks for the review. That makes perfect sense. How would I go about passing the same instance through all the signals without making it a global variable?

avaris commented 4 years ago

Having a module global instance and registering methods of it to signals is fine in this case. Certainly, a lot more preferable to keeping state in files :).

kernc commented 4 years ago

@justinmayer if you can provide some rough test scaffold the way you want to carry it, I'd be happy to add remaining tests.

justinmayer commented 1 year ago

@justinmayer: If you can provide some rough test scaffold the way you want to carry it, I'd be happy to add remaining tests.

I am SO sorry to have taken so long to respond to this. I'm happy to update the CI environment to run pytest, in much the same way as it's done in the relevant section of the Pelican Plugin CookieCutter Template.

Beyond that, I would suggest taking a look at the way other plugins have implemented their tests and then use those as a guide to add tests for this Sitemap plugin.

Does that give you what you need to move this forward so we can get your excellent enhancements merged? 😅

justinmayer commented 1 year ago

@kernc: I have added the necessary test machinery in the CI environment via #20.

justinmayer commented 1 year ago

Hey @kernc! The new test machinery is in place in the CI environment. Any chance you could add some tests and rebase+resolve your enhancements on the current main branch?

justinmayer commented 1 year ago

Hi @kernc! I added some very basic initial tests just to get the CI system to stop failing due to non-existent tests. In addition, #16 has been merged, and Sitemap 1.0.3 has been released, so when you have the time, I look forward to building on top of that and releasing a new version soon containing your excellent refactoring of the Sitemap plugin. ✨

kernc commented 1 year ago

I'm sorry to say I'll still be needing some help re tests that use the detestable tooling I'm unfamiliar with (poetry, pytest). I guess we need to do an integration test with running Pelican proper. Can you point to an example test I can copy/learn from?

justinmayer commented 1 year ago

I'm sorry that you are finding Poetry and Pytest to be unpleasant. What is it about this tooling that you find to be detestable?

Please keep in mind that there is nothing preventing you from using unittest from the Python standard library to write the tests. In fact, most of the tests for the plugins under this Pelican Plugins organization use unittest, with Pytest used only to run the tests.

If you decide to use Pytest methodology instead of unittest to write the tests, the following repositories in this Pelican Plugins organization appear to use Pytest-based tests:

Does that help?

kernc commented 1 year ago

What is it about this tooling that you find to be detestable?

Namely the complexity, compatibility, opinionation (see this idiocy, for instance), and feature creep (see e.g. --help). You'd rightly expect after so many years the development would mature and stabilize, but the charts affirm otherwise. If the projects were any good, CPython would have inherited them by now. After five (or 50 or so) designated build systems (etc.), I'm still waiting for the one we settle on. And I'm now quite confident programming will be completely obsolete before then.

Nevermind the vehemence. I enjoy an opportunity to rant. 😆


OT: I found help in 'markdown-include' addon. Basic covering test added—hope it's fine? One'd sure be challenged to assemble a lighter one with pytest.

Even though the PR changeset balance is now almost at break even, I'll wager the new code is much easier to follow.

justinmayer commented 1 year ago

I appreciate a good rant. 😉 I originally added Poetry to the toolchain because at the time it provided (me) some very real benefits, but over time I have come to the conclusion that it tries to do too much and in a way that does not conform to standards that have coalesced since Poetry was created. Before fully throwing in the towel on some of the benefits provided by more modern tooling, I have done some experiments with Hatchling + PDM and have found that combination to be pleasant as well as more standards-compliant than Poetry. That said, I am still evaluating — the jury remains out.

The “idiocy” you refer to above actually has nothing to do with Poetry, by the way, and is instead the result of CI enforcing linter rules — in this case, Black. One could certainly make a case against Black, of course, but I at least wanted to make sure the vehemence is directed at the right target. 😊

Speaking of linters, I added a commit (018a4c9) to resolve a few things that were causing Ruff to report compliance problems in CI.

justinmayer commented 1 year ago

@kernc: One last question so I can ensure the change-log entry is accurate… When comparing XML files before and after this PR, I noticed the following differences:

Are you aware of any other changes to the output that should be noted in the change-log?

justinmayer commented 1 year ago

Many thanks to @kernc for the significant enhancements and for all the patience along the road to getting this merged. Thanks also to @avaris for reviewing! ✨