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

Settings support for pathlib #2758

Merged
merged 3 commits into from
Oct 28, 2023

Conversation

MinchinWeb
Copy link
Contributor

@MinchinWeb MinchinWeb commented May 22, 2020

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

Copy link
Member

@justinmayer justinmayer left a comment

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
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
Copy link

adnia commented Nov 20, 2023

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
Copy link
Member

avaris commented Nov 20, 2023

@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
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
Copy link

adnia commented Dec 27, 2023

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