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

Allow specifying SRC in pyproject.toml #861

Closed
mwysokin opened this issue May 21, 2019 · 12 comments
Closed

Allow specifying SRC in pyproject.toml #861

mwysokin opened this issue May 21, 2019 · 12 comments
Labels
C: configuration CLI and configuration S: needs discussion Needs further hashing out before ready for implementation (on desirability, feasibility, etc.) T: enhancement New feature or request

Comments

@mwysokin
Copy link

Currently only options can be passed in pyproject.toml, however it would be convenient to pass arguments, specifically SRC . A use case which I have in mind is a gradual migration to black formatting important especially for big projects. It would also allow to track the migration in VCS through changes in pyproject.toml.

It is much more sensible solution than hacking with regex as proposed in #810

e.g. mypy offers such functionality through the files option as described at https://mypy.readthedocs.io/en/latest/config_file.html#config-file-import-discovery-global

@zsol zsol added C: configuration CLI and configuration T: enhancement New feature or request labels May 26, 2019
mrijken pushed a commit to mrijken/black that referenced this issue Aug 13, 2020
@ambv
Copy link
Collaborator

ambv commented Aug 26, 2020

Looking at the PR now, I have concerns to the usability of this design. Are editors supposed to respect this list and treat it as an implicit whitelist, skipping auto-formatting on save for files that aren't on it?

In general, users will be able to run Black with arbitrary paths anyway so this list doesn't protect from that. Today you cannot run black without arguments and there's reasons for that. If people get the experience that it works on some projects but not on others, it will be frustrating and will generate support requests on this tracker from confused users.

Lastly, this seems better served with an external wrapper like pre-commit, Gradle, Makefiles, and so on. If really pushed, the include= option can be used for that. It's actually better in the sense that it supports wildcarding which soon enough would become the next feature requested from src= in pyproject.toml.

@mrijken
Copy link

mrijken commented Aug 30, 2020

Specifying options in pyproject.toml is not different to me as specifying the src path in the .toml file. Both will not confuse users, because the options and the path are explicit part of the config.

Adding the path to ie pre-commit is also not different from adding options to pre-commit.

I do understand your reasoning, but I do not understand why options and src path are different in respect to your reasons.

@felix-hilden
Copy link
Collaborator

Associated PR (because it's not linked yet): #1599

If I'm reading Łukasz and the closed PR right, the main point is that with this change running Black without arguments may or may not work depending on the file configuration. And raising an error with no arguments is a good usability feature. I agree 👍

However, you can already use Black without any arguments by specifying code = 'format="this"' in pyproject.toml. Now I can't imagine anyone ever wanting to do this, but it is possible, and I was actually surprised that it worked. And black src naturally only formats the configuration code.

A couple of points:

  • If specifying source directories in configuration is a no-go, should we restrict code from the configuration too?
  • Could there be any discussion about only allowing a bare black when there actually is some configuration specifying the source?

Personally I could see this working if the output of --verbose and error messages are put in place to leave no question about exactly where the sources are coming from and expected to come from. But I don't know about editor integrations and such.

@ichard26 ichard26 added the S: needs discussion Needs further hashing out before ready for implementation (on desirability, feasibility, etc.) label Jul 3, 2021
@ichard26 ichard26 linked a pull request Jul 3, 2021 that will close this issue
@ichard26
Copy link
Collaborator

ichard26 commented Jul 3, 2021

I am personally -1 about supporting argumentless Black invocations. My main concern comes from a point of safety. While Black is extremely safe regarding code (docstrings are the only exceptions to this - see also #2150 ) due to the checks it implements, rolling back formatting changes due to an accidental Black invocation is rather annoying or impossible.

mypy offers such functionality through the files option as described

Adding the path to ie pre-commit is also not different from adding options to pre-commit.

Mypy is a linter so there's no tangible risk of damage when it's accidentally ran over the wrong files.

Pre-commit's value comes from the use of Git, which provides easy facilities to rollback bad changes so pre-commit's danger is more of a linter than of a formatter. Unfortunately Black can't make the same assumption that the files it's running over are well protected so requiring an explicit source argument is IMO a reasonable requirement.

Also now that Black supports user level configuration it's way easier for a SRC configuration to cause issues (I know we could just block src at the user-level but at that point someone will probably ask for that block to be removed).

If specifying source directories in configuration is a no-go, should we restrict code from the configuration too?

🤣 while I doubt we would ever get an issue about this and therefore am not too happy about having to maintain code to catch this situation, it should be rather cheap so sure?

Finally the other concerns regarding usability and setting and following user expectations are also in my book valid (just imagine expecting to get help output but actually seeing Black format your files by calling black :/ ).

@felix-hilden
Copy link
Collaborator

Rolling back formatting changes due to an accidental Black invocation is rather annoying or impossible.
Mypy is a linter so there's no tangible risk of damage when it's accidentally ran over the wrong files.

Yeah, this is a biggie. Although the changes should be safe, I think I'm turning around just for this.

I doubt we would ever get an issue about [--code in pyproject] and therefore am not too happy about having to maintain code to catch this situation, it should be rather cheap so sure?

I'm quite okay with just leaving it be. It's so weird that it shouldn't be a problem either way.

@felix-hilden
Copy link
Collaborator

felix-hilden commented Jul 5, 2021

And while I see Marc's point about pyproject vs. pre-commit, I think Richard's comment is pretty spot on:

Pre-commit's value comes from the use of Git, which provides easy facilities to rollback bad changes --. Unfortunately Black can't -- assume that the files -- are well protected --.

I might add to clarify, that the existence of a pyproject file does not mean the project is on git.

@mrijken, do you have any other concerns, and what do you think about the points raised here? Since multiple maintainers now agree that this is not the way to go, maybe it would be time to put this issue to rest and close the associated PR, even though it's kind of a bummer.

@ichard26 I'm thinking if there's some documentation work to be done, but I'm not sure if this is appropriate to have on our docs. Maybe in the command line help for SRC, which actually is undocumented currently (even though it's more obvious than the options). Whaddaya reckon?

@mrijken
Copy link

mrijken commented Jul 5, 2021

I'm fine with closing this PR. It would be convenient if the src option can be specified in pyproject.toml, but on the other hand is not a big hassle to add it to the command on the command line.

@mwysokin
Copy link
Author

mwysokin commented Jul 5, 2021

As long as there's an option to specify SRC some way as the original requester I'm 100% fine with closing the issue. TBH I didn't expect that it's going to cause any design discussions. From my point of view it looked like a fairly straightforward and needed feature, but I'm glad that the consensus has been reached. If everyone feels the same way we can close the issue.

@felix-hilden
Copy link
Collaborator

To be clear, currently SRC would be left as a command-line-only parameter! Then a pre-commit configuration can be used to specify the files in a VCS trackable way.

MVrachev added a commit to MVrachev/tuf that referenced this issue Nov 30, 2021
We are using 4 linters: black, isort, pylint and mypy.
It's good if we use one file as a source for truth for all linter
configurations.

As a first step move black options in pyproject.toml.
I tried multiple ways to use the include option,
so we can just call black --config=pyproject.toml, but I was not
successful. Then I found this comment psf/black#861 (comment)
explaining that the path argument is mandatory.
As a result, I will move all configuration options for black inside
pyproject.toml without the actual directories that need to be linted.

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
MVrachev added a commit to MVrachev/tuf that referenced this issue Dec 1, 2021
We are using 4 linters: black, isort, pylint and mypy.
It's good if we use one file as a source for truth for all linter
configurations.

As a first step move black options in pyproject.toml.
I tried multiple ways to use the include option,
so we can just call black --config=pyproject.toml, but I was not
successful. Then I found this comment psf/black#861 (comment)
explaining that the path argument is mandatory.
As a result, I will move all configuration options for black inside
pyproject.toml without the actual directories that need to be linted.

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
MVrachev added a commit to MVrachev/tuf that referenced this issue Dec 1, 2021
We are using 4 linters: black, isort, pylint and mypy.
It's good if we use one file as a source for truth for all linter
configurations.

As a first step move black options in pyproject.toml.
I tried multiple ways to use the include option,
so we can just call black --config=pyproject.toml, but I was not
successful. Then I found this comment psf/black#861 (comment)
explaining that the path argument is mandatory.
As a result, I will move all configuration options for black inside
pyproject.toml without the actual directories that need to be linted.

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
@dbalabka
Copy link

Allowing configuring src in pyproject.toml gives the ability to keep src configurations in the project's repository. Usually, projects have directories that we want to be analyzed by black. It became a bit complicated to keep these configurations in mind and specify in CLI while switching between projects became difficult.

@felix-hilden
Copy link
Collaborator

I'm growing increasingly against this because of the usability problems raised above.

@dbalabka you can already achieve the same with excluding certain folders like we do. Then using black . has the explicitness of knowing that you're about to format the current directory and the safety of ignoring folders you don't want formatted.

@hauntsaninja
Copy link
Collaborator

Closing as per the comments from other maintainers above

@hauntsaninja hauntsaninja closed this as not planned Won't fix, can't repro, duplicate, stale Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: configuration CLI and configuration S: needs discussion Needs further hashing out before ready for implementation (on desirability, feasibility, etc.) T: enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants