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

Normalize arguments loaded from pyproject.toml (#75) #85

Merged
merged 4 commits into from
May 17, 2022

Conversation

ejd
Copy link
Contributor

@ejd ejd commented Apr 30, 2022

This PR converts arguments loaded from a pyproject.toml file into the internal representation used for all other argument sources. This conversion assumes the argument values in pyproject.toml take advantage of TOML types instead of using strings alone. For example, while doc8.ini might have

[doc8]
ignore-path = foo,bar

An equivalent pyproject.toml would have the following:

[tool.doc8]
ignore-path = [
    "foo",
    "bar",
]

@ejd ejd requested a review from ssbarnea as a code owner April 30, 2022 18:39
@ssbarnea ssbarnea added the bug This issue/PR relates to a bug. label May 3, 2022
@ssbarnea ssbarnea changed the title normalize arguments loaded from pyproject.toml (#75) Normalize arguments loaded from pyproject.toml (#75) May 3, 2022
Copy link
Member

@ssbarnea ssbarnea left a comment

Choose a reason for hiding this comment

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

Maybe I am missing something but my impression is that this could be done with a generic conversion from underline to dash instead of an arguably ugly chain for specific cases.

Am I missing something here, like other values that do not respect that rule? Even if I am, it would clearly make sense to create a map with replacements and loop of over it.

@ejd
Copy link
Contributor Author

ejd commented May 3, 2022

I agree that the repetitive try/except blocks are ugly. You are not missing anything -- there are no special cases where an option shouldn't be renamed. I haven't made any contributions to this project before so I chose to base my patch on the from_ini function. I'm happy to change my implementation.

Instead of using the implementation of `from_ini()` as inspiration,
simply loop over the items in the `[tool.doc8]` dictionary and replace
hyphens in names with underscores.

There is a special case for the `ignore-path-errors` argument because
the elements of that list each need to be parsed.

I removed the extra handling for the `extensions` argument which
stripped whitespace from each element in the list.  On reflection, I
realized that the stripping was necessary for INI files due to the way
the `ConfigParser` works and not because it's necessary to guard
against extensions with whitespace.
@ejd
Copy link
Contributor Author

ejd commented May 3, 2022

I removed the mention of from_ini() from my original comment because from_toml() no longer resembles that other function.

@ssbarnea
Copy link
Member

@ejd Can you please add a test? The reason why I did not merge this fix is because I do not have any confidence that it fixes anything and that we will not break it later.

We just need a test for loading the pyproject.toml.

Copy link
Contributor

@stephenfin stephenfin left a comment

Choose a reason for hiding this comment

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

LGTM. As an aside, I find it weird that we don't do "schema validation" of the parsed options (i.e. there are no options that shouldn't be there) but that's not strictly related to this

@ssbarnea
Copy link
Member

If anyone wants to add schema validation, we should produce an JSON Schema and use jsonschema-python project to validate it. I am already doing this on other projects but I am afraid that I do not have the time to do it here.

@ejd
Copy link
Contributor Author

ejd commented May 17, 2022

Can you provide a link to one of those other projects?

@ssbarnea
Copy link
Member

https://github.com/ansible/ansible-navigator validates its own config using schema and https://github.com/ansible/ansible-lint uses schemas to validate various files.

@ssbarnea ssbarnea merged commit 5d785c9 into PyCQA:main May 17, 2022
@ejd ejd deleted the normalize-options-from-pyproject.toml branch May 19, 2022 02:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants