Skip to content

Comments

Custom loader#19

Open
doismellburning wants to merge 27 commits intomasterfrom
feature/custom-loader
Open

Custom loader#19
doismellburning wants to merge 27 commits intomasterfrom
feature/custom-loader

Conversation

@doismellburning
Copy link
Owner

Very much a WIP, but to allow type coercion, setting of defaults, etc.

This changes behaviour - now if ALLOWED_HOSTS is not set in the
environment, you get an empty list; previously you got a list containing
the empty string. This is essentially a bug fix - `[]` is a much more
sensible value than `['']`
(Poorly-worded, but it exists)
@doismellburning
Copy link
Owner Author

@avengerpenguin, @mekhami, @haraball, @jdelic, @carlio, @jvc26 - as users of/contributors to d12f, I'd be very interested in your thoughts and opinions of this feature proposal please!

@doismellburning
Copy link
Owner Author

(Oh, also if you have advice on a) why that code block isn't rendering b) good tools for syntax/style checking of RST, that'd be amazing please)

README.rst Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I think loader should be parser in that constructor

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, that would be significantly better, thanks!

Copy link
Owner Author

Choose a reason for hiding this comment

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

Er, correct-er

README.rst Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you (or could you) provide a "smart bool" option? Simply using bool on a string from os.environ always returns True unless it's empty. Took me ages to figure out why MY_ENV=False was always True in my own stuff :(

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, thanks, I need to publicise/do more about django12factor.getenv_bool!

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps throw some kind of exception or warning if people use parser=bool too, since it's almost certainly not going to do what they want. Or, in the spirit of heroku-fucking-console, just use getenv_bool if they pass in a bool.

@carlio
Copy link
Contributor

carlio commented Jan 17, 2015

I think "For example:" needs 2 colons after to get the code bock to render. Not sure though as I don't use the ..code-block directive in my stuff.

@doismellburning
Copy link
Owner Author

Heh cheers, I thought that, but it seems identical to earlier codeblocks in my rst...

@carlio
Copy link
Contributor

carlio commented Jan 17, 2015

Possible alternative syntax:

custom_settings = EVL(
  ("LISTEN_PORT", 8080, int),
  ("OTHER_VAL", "default"),
)

Not sure if that is nicer or not, basically just removes lots of EVL, but could lead to parsing faff.

@doismellburning
Copy link
Owner Author

Yeah I'm not sure how much I like the syntax, but I've not found a happier alternative - tuples make it a pain to have optional arguments; dicts might be better, but basically I've taken a leaf from Django's url...

@doismellburning
Copy link
Owner Author

Fun observations: DATABASE_URL is a bit of a faff because it breaks the "setting name == env var name" assumption.

For customisable defaults you can do something like:

evl = EVL(
    "DATABASE_URL", 
    parser=dj_database_url.parse,
    default=dj_database_url.parse("your://url")
)
DATABASES['default'] = evl.load_value()

which is icky but I'm not sure how best to improve it

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 3d69fc3 on feature/custom-loader into 75c1d75 on master.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling d8d7bbe on feature/custom-loader into * on master*.

@rfleschenberg
Copy link

Is there anything I can do to move this PR, or django12factor in general, forward?

I have stopped using it for now and switched to a combination of os.environ.get(), dj-database-url and custom helper functions for parsing env vars, but it seems to be a waste of time if people keep reinventing this, even if it's quite simple.

@rfleschenberg
Copy link

rfleschenberg commented Nov 18, 2016

I realize this idea is somewhat radical, but do you think we could have a "just a bunch of functions" API?

DEBUG = d12f_bool('DEBUG', default=True)

SECRET_KEY = d12f_string('SECRET_KEY', require=True)  # raise an exception if not set.

DATABASES['default'] = d12f_database_url('DATABASE_URL', default='your://url')

LOGGING = d12f_logging()

WHATEVER = d12f_custom('MY_ENV_VAR', parser=lambda s: s.lower())

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.

4 participants