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

Only allow snake_case keys in configuration file (#4753) #4754

Merged
merged 2 commits into from Mar 19, 2018

Conversation

Projects
None yet
2 participants
@ceh
Contributor

ceh commented Mar 17, 2018

We must use the original key when reading the value from the config file, since it may have been specified in a dash-notation.

Fixes #4753.

@gvanrossum

This comment has been minimized.

Member

gvanrossum commented Mar 17, 2018

It looks like the code never worked. Seeing how many places would have to be modified, it feels brittle to try and support this. I suggest that it may be better to just get rid of orig_key and delete the normalization at line 764. We may have to update the docs too.

@gvanrossum

Thanks! I'll merge it Monday.

@ceh

This comment has been minimized.

Contributor

ceh commented Mar 17, 2018

I couldn't find any documentation where it's stated that the dash-notation is supported. See e.g. https://github.com/python/mypy/blob/10522cf/docs/source/config_file.rst

I added a test case as well, so that it's documented in some sense that it's not a valid syntax.

@gvanrossum

This comment has been minimized.

Member

gvanrossum commented Mar 18, 2018

And thanks for all that! Agreed, it wasn't even documented (I think it was just a wishful feature in my mind when I wrote the code).

@ceh ceh changed the title from Use original key when reading value from config file (#4753) to Only allow snake_case keys in configuration file (#4753) Mar 18, 2018

@gvanrossum

This comment has been minimized.

Member

gvanrossum commented Mar 19, 2018

And thanks for all that! Agreed, it wasn't even documented (I think it was just a wishful feature in my mind when I wrote the code).

@gvanrossum gvanrossum merged commit 539087e into python:master Mar 19, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ceh ceh deleted the ceh:issue-4753 branch Mar 24, 2018

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