-
Notifications
You must be signed in to change notification settings - Fork 46
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
Convert bool values from a .ini file #245
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution @hiromu! As mentioned we need to make sure this is forwards-compatible. Also, how were you specifying config values that you ran into this issue? bravado-core itself doesn't use asbool
.
@@ -217,12 +218,13 @@ def create_bravado_core_config(settings): | |||
'use_models': False, | |||
} | |||
configs.update({ | |||
bravado_core_key: settings[pyramid_swagger_key] | |||
bravado_core_key: (settings[pyramid_swagger_key] if bravado_core_key == 'formats' | |||
else asbool(settings[pyramid_swagger_key])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is dangerous, as any new bravado-core config key which doesn't have a boolean value would break this. We should instead list the bravado-core keys we know of and the function we should use for each key to get the value. Ideally we'd provide an API to do this within bravado-core.
for pyramid_swagger_key, bravado_core_key in iteritems(config_keys) | ||
if pyramid_swagger_key in settings | ||
}) | ||
configs.update({ | ||
key.replace(BRAVADO_CORE_CONFIG_PREFIX, ''): value | ||
key.replace(BRAVADO_CORE_CONFIG_PREFIX, ''): asbool(value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't handle formats
.
I understand your concerns on the forward compatibility. Are you considering to implement an API within bravado-core? Otherwise, I'll update this pull request so as to use a hard-coded allowlist to determine whether to apply
In my implementation, I'm specifying |
When I use a .ini file for configuration, some variables are passed to bravado-core in the form of str and always treated as
True
.Since all variables except
formats
are boolean, they should be converted usingasbool()
: https://bravado-core.readthedocs.io/en/stable/config.htmlThe same thing happens for
enable_api_doc_views
in__init__.py
, and thus I fixed.