Skip to content

Settings support for pathlib - #2758

Merged
justinmayer merged 3 commits into
getpelican:masterfrom
minchinweb:settings-pathlib-2
Oct 28, 2023
Merged

justinmayer merged 3 commits into
getpelican:masterfrom
minchinweb:settings-pathlib-2

Conversation

@minchinweb

@minchinweb minchinweb commented May 22, 2020 •

Copy link
Copy Markdown
Contributor

Further to the work I did in PR #2738 (moving all settings to pathlib.Path).

It was decided that it was better to keep the default settings as strings, but this is the code that allows Path's to provided for any of these settings.

Perhaps it makes sense to add this support (since the work is done already), so I've extracted the support code.

  • 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
  • [ ? ] Updated documentation for changed code

@justinmayer justinmayer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks to @minchinweb for the enhancement (and the patience) and also to @avaris for reviewing. 👏

@justinmayer
justinmayer merged commit e14f20b into getpelican:master Oct 28, 2023
@minchinweb

Copy link
Copy Markdown
Contributor Author

Delighted to see it merged (and useful) after all this time!

@minchinweb
minchinweb deleted the settings-pathlib-2 branch October 28, 2023 20:11
@adnia

adnia commented Nov 20, 2023

Copy link
Copy Markdown

I believe this change breaks the Dynamic Pelican Settings workaround that I also relied on. I also had the use case of preserving old link structures from before switching to Pelican to the best extent possible.

Especially this change seems to be the culprit as it now returns str(<class>).format(...), i.e, dynamically overwriting the format() function is no longer possible.

Indeed, changing the logic to str(<class>.format(...)) resolves the issue for me. Is there a better, intended solution instead of using this workaround?

@avaris

avaris commented Nov 20, 2023 •

Copy link
Copy Markdown
Member

@adnia from a brief glance, for that to work you also need to define what str(...) returns, I believe this would work.

class DynamicSetting(str):
    def __init__(self, f):
        self.f = f
    def format(self, **metadata):
        return self.f(metadata).format(**metadata)
    def __str__(self):
        return self

But, I have to emphasize that this method of monkey-patching, although neat, still relies on certain structure for things that are "internal" to pelican. So, I cannot guarantee that things won't change in the future :).

EDIT: Need to inherit from str. This is weirdly hacky. I'll update if I figure something out.

However, "proper" way to handle this would be a plugin. You just need to connect to content_object_init and set override_save_as and override_url values. Something like

from pelican import signals

def update_save_as_url(content):
    # logic can be extended to differentiate pages and articles based on content class
    if "cleanurl" in content.metadata:
        content.override_save_as = '...'
        content.override_url = '...'

def register():
    signals.content_object_init.connect(update_save_as_url)

@minchinweb

Copy link
Copy Markdown
Contributor Author

Indeed, changing the logic to str(.format(...))

This doesn't work, as the logic needs to work on both Pathlib.path and str. In particular, .format() is a function of the str class, so you need to have a string before you can format it.

The plugin idea is probably a good way to go; if you get stuck, ask here, I've written a number of plugins over the years. :)

@adnia

adnia commented Dec 27, 2023

Copy link
Copy Markdown

Just as a quick, yet belated update: I finally came around adjusting my Pelican setup. I noticed that I had unsuccessfully tried to incorporate the handling of this corner case in my larger plugin. However, with the the pointers provided by @avaris, I was able to adjust my code and succeed, ditching the workaround. :)
So, thank you everyone for the support. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

4 participants