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

feat(config): Support --config in pyproject.toml #2525

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

Shivansh-007
Copy link
Contributor

@Shivansh-007 Shivansh-007 commented Oct 7, 2021

Description

You can now specify config key in pyproject.toml which does the same job as the CLI option --config. The path of config should be relative to the CWD it is running from. If the file is not found or is invalid i.e. it is a folder rather a TOML file it would raise click.exceptions.FileError, and if the file can't be parsed it would raise tomli.TOMLDecodeError. All black config keys in the custom config should be registered under [black] and they would overwrite the config specified in pyproject but not the one specified by --config flag while running black with the CLI.

I also did a small update to .gitignore by grouping the "test" and adding htmlcov to it. Which is generated by coverage html and .pytest_cache/ produced when you run pytest ..

Closes #1826

Checklist - did you ...

  • Add a CHANGELOG entry if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

@felix-hilden
Copy link
Collaborator

Thanks for getting this started as well!

Couple of discussion points:

  • I think a path relative to the original TOML's location should be better, right? That way the run wouldn't depend on where you executed Black, as it is currently.
  • How about just overriding the configuration instead of trying to merge it?

@ichard26 ichard26 self-requested a review October 7, 2021 15:49
@ichard26
Copy link
Collaborator

ichard26 commented Oct 7, 2021

I haven't taken a deep look at the code nor played with the feature locally but I do quickly want to mention we should probably somehow indicate we using a configuration file via indirection in verbose mode. Honestly I'm not sure what's up with the weird merging / overriding behaviour here, but I'd suggest to keep it simple for now and just treat a config value in config file (regardless if was explicitly given or implicitly discovered) as a HTTP 302 redirect.

@Shivansh-007
Copy link
Contributor Author

Question: Should the config be overwritten if it is passed through the CLI option --config? The config = "black.toml would be found in that file (which is passed through the CLI).

@ichard26
Copy link
Collaborator

ichard26 commented Oct 8, 2021

I'm confused by what you mean, can you further explain what scenario we're talking about here? Do you mean whether --config at the command line should override just the config value in the implicitly discovered configuration file or instead override the use of that file? In that case I'd suggest to ignore any implicitly discovered configuration files as that's what passing --config at the command line does today.

@Shivansh-007
Copy link
Contributor Author

There are two ways configs are loaded, (1) they are passed by the user through --config flag and (2) the pyproject.toml is found in the src. So currently when we find the config key we check whether if it is found by the config found in pyproject.toml or it is found in the one passed by the user via --config option. I am not sure why I did this originally, so I was asking should I keep this check or just overwrite it in any case. I would go with the latter.

@ichard26
Copy link
Collaborator

ichard26 commented Oct 8, 2021

So currently when we find the config key we check whether if it is found by the config found in pyproject.toml or it is found in the one passed by the user via --config option

Ah this is relevant because you handled those two cases differently. I'd say treat them the same as a HTTP 302 redirect but for configuration for simplicity. If the config key was found in pyproject.toml, ignore the rest and switch to the file specified. Same thing with the --config option, if present, just start the configuration loading process at its filepath. I feel like adding another merging rule on top of the "CLI options override options defined in a configuration file" merge rule will just overcomplicate things. Afterall --config is intended as an explicit way to force Black to use a specific configuration file (instead of searching for one) and nothing more. If the configuration file specified by --config contains a config key, then switch configuration files, kinda like a game of hot potato :p

Also you know we're on Python Discord right? It might be easier to discuss in the #black-formatter channel ^^

@Shivansh-007
Copy link
Contributor Author

Shivansh-007 commented Oct 8, 2021

Yeah, I am on discord also (Jason Terror#XXXX) but currently, off from it until my exams ended, I have a break between the next ones till the 17th so opened these PRs - didn't want to keep them hanging, school ones just finished. And discord as you know, can become a big distraction LOL

Copy link
Collaborator

@felix-hilden felix-hilden left a comment

Choose a reason for hiding this comment

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

Richard also raised a good point about chaining this value in the original issue. I tend towards chaining being supported, which would require a loop of some sort to follow the TOML files.

Also, I think it would be more consistent to just require all TOMLs to have the same structure, i.e. a tool.black section instead of some other magical section. That way we could probably simplify the implementation and also solve chain cycles by looping through the files with an iteration threshold.

@Shivansh-007
Copy link
Contributor Author

By chaining do you mean you can specify config in the custom black config file you got from pyproject.toml, and so on (allowing to specify config in that config file...)?

Copy link
Collaborator

@felix-hilden felix-hilden left a comment

Choose a reason for hiding this comment

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

Looking better already, here's some comments:

src/black/__init__.py Outdated Show resolved Hide resolved
tests/test.toml Outdated Show resolved Hide resolved
tests/invalid_test.toml Outdated Show resolved Hide resolved
tests/test_black.toml Outdated Show resolved Hide resolved
@Shivansh-007 Shivansh-007 force-pushed the feat/pyproject-config branch 3 times, most recently from 0c516ef to 88c80c2 Compare October 21, 2021 00:54
@ichard26 ichard26 added the C: configuration CLI and configuration label Oct 22, 2021
CHANGES.md Outdated Show resolved Hide resolved
tests/test_black.py Outdated Show resolved Hide resolved
tests/test_black.py Show resolved Hide resolved
docs/usage_and_configuration/the_basics.md Outdated Show resolved Hide resolved
docs/usage_and_configuration/the_basics.md Outdated Show resolved Hide resolved
You can now specify `config` key in pyproject.toml which does the same
job as the CLI option `--config`. The path of config should be relative
to the CWD it is running from. If the file is not found or is invalid
i.e. it is a folder rather a TOML file it would raise
`click.exceptions.FileError`, and if the file can't be parsed it would
raise `tomli.TOMLDecodeError`. All black config keys in the custom
config should be registered under `[black]` and they would overwrite the
config specified in pyproject but not the one specified by `--config`
flag while running black with the CLI.

Closes psf#1826
There are two ways configs are loaded, (1) they are passed by the user through `--config` flag and (2) the `pyproject.toml` is found in the `src`. So currently when we find the `config` key we check whether if it is found by the config found in `pyproject.toml` or it is found in the one passed by the user via `--config` option.

In this commit, the above check has been dropped, and if config key found in any of the config, it would overwrite it no matter what.
Copy link
Collaborator

@ichard26 ichard26 left a comment

Choose a reason for hiding this comment

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

I did not do a hands-on test as I'm far too annoyed right now to do so but here's some initial feedback. We also need to check the pypy coverage drop.

docs/usage_and_configuration/the_basics.md Outdated Show resolved Hide resolved
docs/usage_and_configuration/the_basics.md Outdated Show resolved Hide resolved
docs/usage_and_configuration/the_basics.md Outdated Show resolved Hide resolved
docs/usage_and_configuration/the_basics.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
@JelleZijlstra
Copy link
Collaborator

This has a merge conflict without an obvious fix, could you take a look?

@Shivansh-007 Shivansh-007 force-pushed the feat/pyproject-config branch 2 times, most recently from 491b467 to 092027b Compare March 11, 2022 14:40
@JelleZijlstra
Copy link
Collaborator

@ichard26 are you happy with this PR now?

@danielmitchell
Copy link

This PR appears to have stalled? It would be great if it could progress since there are a couple of related issues (#1826, #2863) it will resolve and make black much easier to configure in a monorepo.

@ichard26
Copy link
Collaborator

Hey all, I'll try to review this soon (especially since IIRC I had some outstanding comments I never shared the last time I looked at this), but I can't make promises.

@jaklan
Copy link

jaklan commented Jul 29, 2022

@ichard26 any chance to push it forward?

@AbdealiLoKo
Copy link

I was looking for the usecase described in #1826 and came across this PR
Would be great if this can be released!

If I can help with something to push this forward, I'm happy to help

@alfawal
Copy link

alfawal commented Jul 29, 2023

@ichard26 Just a reminder to take a look please.

@hauntsaninja
Copy link
Collaborator

See also #1826 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: configuration CLI and configuration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for --config in pyproject.toml
9 participants