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

--setting_module Option #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

--setting_module Option #8

wants to merge 1 commit into from

Conversation

198d
Copy link

@198d 198d commented Dec 14, 2012

I have a little library here at work (configtools) that helps me mange Django settings in config files and making a module kind of act like django.conf.settings by pulling config from some other preconfigured module (considering open-sourcing it, but that's beside the point). It makes use of something like sys.modules[name] to get access to the current module object at import time. Unfortunately, the way Logan handles the --config option and loading the file breaks that. This new option takes a dotted module name that gets passed almost directly to import fixing my problem.

I'd almost recommend replacing --config with --settings_module as, with my experience in the python community, most always you refer to a module or function in it's dotted form (e.g. package.module or package.module:function) when passing them as arguments to command line tools (see DJANGO_SETTINGS_MODULE, how you launch an app with gunicorn, etc...). This keeps backwards compatibility with --config and prevents both from being passed at the same time.

Allow logan apps to accept a --settings_module option that takes a
dotted string representing an import path (e.g. my_module.settings).
This value eventually gets run through __import__ and makes it possible
to look up the module object by __name__ in sys.modules while loading
the config for the logan app.
@198d
Copy link
Author

198d commented Dec 14, 2012

Taking a second look at the diff, I might be overloading the mod_or_filename param to load_settings a little too much. The assumption that an import error means the value is a filename might hide the problem when the value is supposed to be a module name but is just wrong.

@dcramer
Copy link
Owner

dcramer commented Dec 14, 2012

Unless I'm misunderstanding this, the point of config is that you can actually pass a file path (instead of a module). Given that case I don't see much of a point in incorrectly renaming it.

If there's actual broken behavior then we should definitely fix that

On Dec 14, 2012, at 9:25 AM, John MacKenzie notifications@github.com wrote:

I have a little library here at work (configtools) that helps me mange Django settings in config files and making a module kind of act like django.conf.settings by pulling config from some other preconfigured module (considering open-sourcing it, but that's beside the point). It makes use of something like sys.modules[name] to get access to the current module object at import time. Unfortunately, the way Logan handles the --config option and loading the file breaks that. This new option takes a dotted module name that gets passed almost directly to import fixing my problem.

I'd almost recommend replacing --config with --settings_module as, with my experience in the python community, most always you refer to a module or function in it's dotted form (e.g. package.module or package.module:function) when passing them as arguments to command line tools (see DJANGO_SETTINGS_MODULE, how you launch an app with gunicorn, etc...). This keeps backwards compatibility with --config and prevents both from being passed at the same time.

You can merge this Pull Request by running:

git pull https://github.com/198d/logan master
Or view, comment on, or merge it at:

#8

Commit Summary

--settings_module Option
File Changes

M logan/importer.py (25)
M logan/runner.py (38)
M logan/settings.py (19)
Patch Links

https://github.com/dcramer/logan/pull/8.patch
https://github.com/dcramer/logan/pull/8.diff

Reply to this email directly or view it on GitHub.

@198d
Copy link
Author

198d commented Dec 14, 2012

Unless I'm misunderstanding this, the point of config is that you can actually pass a file path (instead of a module).

That is correct. The --config option as it is works completely as advertised. Unfortunately the way the code in that file is evaluated (via execfile) doesn't work with the settings module I'm to run Sentry with. My proposal for the --settings_module option is just an alternate way of specifying the config for the Logan app. It allows the user to specify their config as a dotted module path (e.g. similar to how you'd specify the DJANGO_SETTINGS_MODULE environment variable) and then allows load_settings to properly import (via __import__) that value.

That second paragraph in the original request was purely philosophical; I was just commenting on how most Python CLI tools I've interacted with usually take the dotted form of a module and import it rather than the path to the file. I avoided breaking --config as I figure it's best to let you make that call.

@198d
Copy link
Author

198d commented Mar 4, 2013

Have you given any more thought on this? I'm maintaining an internal fork of logan that I'd like to get rid of. The few people I've talked to about this have thought it was a good idea and further criticized the way in Sentry, for example, you have to pass a path to a config file rather than a module name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants