Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

A Possible Re-implementation for Issue #445 #449

Merged
merged 2 commits into from Aug 16, 2012

Conversation

Projects
None yet
2 participants

Julian commented Aug 1, 2012

This reimplementation of settings loading will make settings modules more like normal modules (in particular, it preserves __file__, which is the most important thing there, and most likely to be used in a settings module.

Mentioning the issue here so GitHub picks up on it:

Closes #445

I also should note that in the implementation you had (and also here, I didn't change anything behaviorally other than the added functionality, at least not according to your test suite :), you are only shallow copying the default settings. You've got some mutable data structures in there, so, if for some reason someone manages to run pelican twice with different settings in the same process there can be issues there. That probably deserves an issue ticket if you care about that use case.

Owner

almet commented Aug 16, 2012

This looks good to me, and I prefer this implementation rather than the previous one, so thanks, that's elegant :-)

You're right about the fact that we're returning the mutables directly here. One way to avoid this is to deep-copy these settings when reading them. That's not an intended use case of pelican as a tool, but can still be useful to have if we're using pelican as a lib at some point, thanks for pointing this out.

@almet almet added a commit that referenced this pull request Aug 16, 2012

@almet almet Merge pull request #449 from Julian/settings
A Possible Re-implementation for Issue #445
e4df1da

@almet almet merged commit e4df1da into getpelican:master Aug 16, 2012

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