feat(config): Add support for path specific config #2527
Conversation
Woohoo! Module specific configuration! and a bit of restructuring of code in the
`black/__init__.py` file. Mainly shifting all the "reformatting" logic to a
separate function to increase the readability of code in the click CLI
function. You can go through the code in the commit, code should be self
explanatory, though I still need to update the documentation of the
code and this feature.
I haven't aimed at bringing new tests in this commit, if required, I
could do that and this rather fixes the tests to work according to the
change I have done in this commit.
Summarising the changes:
- Now, you can support file/module specific configuration in the
pyproject.toml config under the section of `[tools.black-module]`. I
have added a separate "key" for the modules as according to the
current code when the module name could clash with a config argument.
For example `[tools.black.src]` would clash with `SRC`.
- Module specific configuration can only some arguments which are listed
under a global constant in `config.py` and should agree with the
type hint present on them in the class variables, the logic for this
functionality can be seen in `cast_typehint` function of `config.py`.
At some places I have ignored MYPY errors so as I couldn't change the
configuration since they were being used for typing.
- Added a new class for storing all the parameters, the order of the
arguments is as follows:
* Read global pyproject.toml/specified config config.
* Read module specific config, which would take the none specified
arguments from the global configuration.
* If value specified through click options/arguments it would be
overwritten, this won't happen if the default value of the option is
taken.
* Tada!
|
Hi and thanks for the submission! But I'll be honest, I'm not sure being able to specify a different value for string normalisation, trailing commas and such in subdirectories is a great idea. I can sort of see it for the documentation line length use case presented in the linked issue, but this really clashes with our consistency and non-configurability ethos. So I'm not sure if practicality beats purity here. Second opinions? |
|
That was one point of discussion I forgot to include in the PR description. I could change the values accordingly once decided. |
|
Hello! I will try to do a more thorough review eventually but I do some initial feedback: FWIW, it would've been a good idea to get sign off on the original issue before implementing this as this has some ethos design considerations which could easily block it. And for the next contribution I'd encourage putting refactor changes in their own preceding commits so I don't have to spend too much time discerning what changes are for what. Even if code is self-explanatory, that doesn't fix the problem of that it's simply hard to review a big change without clear sections. Also, what's up with the Pipfile.lock changes? They don't look very correct to me as many dependencies needed on 3.6/7 seem to have been dropped (you can blame pipenv's strong dependence on the runtime version during locking for that 🙃) but I also don't see a reason why Pipfile.lock needs to be touched here. If it's a routine upgrade you're trying to do, I'd suggest moving them to a new PR (although honestly I'd suggest not trying to touch pipenv's lockfiles as it's a shockingly annoying thing to use). Thanks for the PR though! |
|
Right, I was on an earlier commit while working on this and the I do keep my commits atomic for most of my PRs, it's just I saw squashed commits being merged to main. On a few PRs originally people were force pushing their changes to a single commit after addressing the reviews, so I thought it was some sort of a policy to keep the changes squashed. |
We don't really care either way as we prefer squash merges into main regardless of how the branch's history. The key point here is "after addressing the reviews". We prefer to squash during the final merge but reviewing well defined commits (although it's a balancing act between the changes being one big pile vs a giant mess of too many small commits) is so much easier. If anything people shouldn't be force-pushing until we're about to merge but oh well1 🤷. Footnotes
|
|
Alright, will remember that! |
|
Just to be clear, my initial gut feeling would be to say no unless we really narrow down the possible configurations. I'd be okay with exclude = [
"docs/src/examples",
"...",
]
line-length = [
["docs/src/include", 65],
["...", 9000]
]Or whatever the appropriate TOML syntax is. Although to be fair the regex would be able to handle "multiple values" for exclusion as well 😄 And I'm not saying we should do this either! But if we're going down this route for a narrow subset of configuration values, I think this could be a better way. |
|
I like that style to be honest 💯 Though while working on this, I had aimed it to be similar to the |
|
Yeah, I guess in hindsight it would have been better to discuss further on the issue since it's so old 😅 |
|
This has a bunch of merge conflicts now, and there's still no consensus yet on whether we should even do this (but that should be discussed on the issue). Are you still interested in moving this forward? |
|
Yeah, I have been waiting for the design approval, whether we should go with what Felix proposed or the current solution. |
ichard26
added
the
S: blocked
|
You |
|
Given the discussion on the issue, I'm going to close this for now. Thanks for your contribution! |
Lol, I am sorry for spamming 3 PRs. Had completed them before vacation and didn't get time to open them until now :)
Description
Woohoo! Module specific configuration! and a bit of restructuring of code in the
black/__init__.pyfile. Mainly shifting all the "reformatting" logic to a separate function to increase the readability of code in the click CLI function. You can go through the code in the commit, code should be self explanatory, though I still need to update the documentation of the code and this feature.I haven't aimed at bringing new tests in this commit, if required, I could do that and this rather fixes the tests to work according to the change I have done in this commit.
Summarising the changes:
Now, you can support file/module specific configuration in the pyproject.toml config under the section of
[tools.black-module]. I have added a separate "key" for the modules as according to the current code when the module name could clash with a config argument. For example[tools.black.src]would clash withSRC.Module specific configuration can only some arguments which are listed under a global constant in
config.pyand should agree with the type hint present on them in the class variables, the logic for this functionality can be seen incast_typehintfunction ofconfig.py. At some places I have ignored MYPY errors so as I couldn't change the configuration since they were being used for typing.Added a new class for storing all the parameters, the order of the arguments is as follows:
Closes #1370
Checklist - did you ...