Re-factor Sitemap plugin #3
Conversation
* Act on every `content_written` signal to avoid guessing what pages to cover. * Makes it work correctly with i18n_subsites plugin (before, it created a separate sitemap file for each subsite). * Don't manually fiddle with timezones. Instead, expect `article.date` to be TZ-aware if required. * `settings['SITEURL']` is modified within `content_written` signal. With `RELATIVE_URLS=True`, the resulting advertised file locations are likely incorrect.
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? |
|
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 :). |
| trans.lang, | ||
| siteurl, | ||
| # save_as path is already output-relative | ||
| clean_url(pathname2url(trans.save_as)), |
There was a problem hiding this comment.
This now works with i18n_subsites (:tada:), but here the produced URL in translation links is incorrect. trans.save_as is formatted according to e.g. PAGE_LANG_URL for all subsites (including the DEFAULT_LANG subsite where PAGE_URL is the one appropriate).
Ideas on how else to get the proper final URL/path on disk?
There was a problem hiding this comment.
Can someone else confirm this? i18n_subsites no longer build with my project. 😬 😂
|
@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 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? 😅 |
|
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 |
|
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. ✨ |
|
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'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 If you decide to use Pytest methodology instead of
Does that help? |
Namely the complexity, compatibility, opinionation (see this idiocy, for instance), and feature creep (see e.g. Nevermind the vehemence. I enjoy an opportunity to rant. 😆 OT: Even though the PR changeset balance is now almost at break even, I'll wager the new code is much easier to follow. |
|
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. |
|
@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? |
This is an attempt at PoC simplification (ymmv) of the plugin. Comments and ideas welcome!
content_writtensignal to avoid guessing what pages to cover.Makes it work correctly with i18n_subsites plugin (before, it created a separate sitemap file for each subsite).Ok, this actually doesn't work. No idea what I was testing before. 😢article.dateto be TZ-aware if required.settings['SITEURL']is modified withincontent_writtensignal. WithRELATIVE_URLS=True, the resulting advertised file locations are likely incorrect.