Skip to content

Custom loader - #19

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

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

Conversation

@doismellburning

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

Comment thread README.rst Outdated

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

Comment thread README.rst Outdated

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

carlio commented Jan 17, 2015

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Owner Author

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

@carlio

carlio commented Jan 17, 2015

Copy link
Copy Markdown
Contributor

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

Coverage Status

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

@coveralls

Copy link
Copy Markdown

Coverage Status

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

@rfleschenberg

Copy link
Copy Markdown

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

rfleschenberg commented Nov 18, 2016

Copy link
Copy Markdown

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