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

Convert (most of the) configuration values from pyproject.toml to strings #1466

Merged
merged 3 commits into from
Jun 1, 2020

Conversation

ichard26
Copy link
Collaborator

Resolves #1458.


We need to convert all configuration values from the pyproject.toml
file because Click already does value processing and conversion and
it only expects the exact Python type or string. Click doesn't like
the integer 1 as a boolean for example. This also means other
unsupported types like datetime.time will be rejected by Click as
an invalid value instead of throwing an exception.

We only skip converting objects that are an instance of
collections.abc.Iterable because it's almost impossible to get back
the original iterable of a stringified iterable.


After this change, these following values will be accepted for each of the configuration options in pyproject.toml:

Option Accepted values
Boolean type / flag type
  • --pyi
  • -S / --skip-string-normalization
  • --check
  • --diff
  • --color / --no-color
  • --fast / --safe
  • --quiet
  • --verbose
  • Integers
    • 0
    • 1
  • Strings (case insensitive)
    • "f"
    • "false"
    • "n"
    • "no"
    • "0"
    • "t"
    • "true"
    • "y"
    • "yes"
    • "1"
  • Boolean
    • true
    • True (technically a string)
    • false
    • False (technically a string)
Integer type
  • -l / --line-length
  • Any integer
  • Any stringified integer
Choice type
  • -t / --target-version
The choices available for that option.
Text type
  • -c / --code
  • --include
  • --exclude
  • --force-exclude
Any text still accepted.

And --version, --help, and --config are still allowed in pyproject.toml 😆

We need to convert all configuration values from the pyproject.toml
file because Click already does value processing and conversion and
it only expects the exact Python type or string. Click doesn't like
the integer 1 as a boolean for example. This also means other
unsupported types like datetime.time will be rejected by Click as
a unvalid value instead of throwing an exception.

We only skip converting objects that are an instance of
collections.abc.Iterable because it's almost impossible to get back
the original iterable of a stringified iterable.
@ichard26
Copy link
Collaborator Author

I went with collections.abc.Iterable since I don't really know what iterable types the TOML parser might return. If it is overkill, I could just replace it with list since from my limited testing, that is the only iterable the TOML parser returns (well other than strings).

Instead of converting the values in the merged 'default_map', I should
convert the values that were read from the 'pyproject.toml' file.
Copy link
Collaborator

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

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

I think toml and is like JSON and only has lists and dicts right? So list would probably be safe, but so would be Iterable. So I don’t have any strong opinion.

I also moved where the conversion happens... again. I am rather indecisive
if you haven't noticed. It should be better as it takes place in the
parse_pyproject_toml logic where configuration modification already takes
place.

Actually when this PR was first created I had the conversion happen in that
return statement, but the target_version check was complaining about it being
a string. So I moved the conversion after that check, but then Click didn't
like the stringifed list, which led me to check whether the value was an
instance of an Iterable before turning it into a string. And... I forgot that
type checking before conversion would allow it to work before the
target_version check anyway.
@ichard26
Copy link
Collaborator Author

ichard26 commented Jun 1, 2020

Well, I have no experience with JSON, but your argument makes sense. The TOML parser shouldn't change what it returns that much since it does follow the TOML specification, which shouldn't change that often. I've changed it to (list, dict) just to be safe.

@ichard26
Copy link
Collaborator Author

ichard26 commented Jun 1, 2020

Completely unrelated, but the primer check is still a bit flaky on Windows, this run preferred to fail apparently.
https://github.com/ichard26/black/runs/728070221?check_suite_focus=true

Isn't much of a problem though, it seems to be always an issue with the event loop so it's easy to spot.

EDIT: oh, excuse me?

PermissionError: [WinError 32] The process cannot access the file because it is being used by another process

@cooperlees
Copy link
Collaborator

EDIT: oh, excuse me?

PermissionError: [WinError 32] The process cannot access the file because it is being used by another process

I guess we should open a dedicated issue for this. We can always stop Windows as 99% of value is finding logic area in black, it shouldn't be Windows specific, but, still, always nice to check on all platforms.

@cooperlees cooperlees merged commit d10f857 into psf:master Jun 1, 2020
@ichard26 ichard26 deleted the improve_configuration_handling branch April 12, 2021 01:21
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.

AttributeError: 'int' object has no attribute 'lower'
2 participants