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

Configuration #65

Closed
spapanik opened this issue Mar 22, 2018 · 23 comments
Closed

Configuration #65

spapanik opened this issue Mar 22, 2018 · 23 comments
Labels
T: style What do we want Blackened code to look like?

Comments

@spapanik
Copy link

It would be nice if there was the option to store black's line length and --safe/--fast options in setup.cfg

@ambv
Copy link
Collaborator

ambv commented Mar 22, 2018

I keep the number of command-line options to black super small to avoid having to have a configuration file. If you support configuration files, you have to look for them. Since you already mentioned setup.cfg, maybe we want a black.ini, too. Which one takes precedence? And so on, and so on. After my experience with Flake8 and what it's doing, I'm inclined to avoid this problem altogether for as long as we can.

Would a shell alias work for you instead?

alias black79="black --line-length=79 --fast"

@ambv ambv added the T: style What do we want Blackened code to look like? label Mar 22, 2018
@Kos
Copy link

Kos commented Mar 23, 2018

mild +1

The way I see it, the main benefit from config files for formatters is to be able to commit them to the repo.
If there's no config, people would need a work-around, for example

#!/bin/sh
black --line-length 79 --fast "$@"

and then remember to say ./black instead of black.

Then you'll have editor integrations (format on save). Ideally you could just have:

  • editor calls black from PATH
  • black picks up settings from config

If there's no config, then each dev needs to set a custom path to the black binary in their editor's plug-in (assuming the plugin author remembered about this option) in order to use the project's configuration.

Long story short, having a config helps with having uniform configuration in a project and saves a few workarounds.


OTOH: all this for --line-length and --fast? Maybe a better idea would be to pick up preferred line length from an existing config file (flake8/pycodestyle), instead of inventing a new one?

@njsmith
Copy link

njsmith commented Mar 30, 2018

If you do have a config file, don't use setup.cfg, use pyproject.toml (under the [tool.black] table).

@ambv
Copy link
Collaborator

ambv commented Mar 30, 2018

It's disappointing the PEP doesn't mention setup.cfg at all. It's not clear what the advantage is and doesn't address migration of tooling configuration from the old file to the new file.

@njsmith
Copy link

njsmith commented Mar 30, 2018

From the perspective of a tool like black, the advantages are (1) TOML is a much nicer format than .cfg, (2) no one has to use setup.cfg, but many projects will have to use pyproject.toml. In particular, it's the only working way to specify build-time dependencies, and it will be where projects specify which build system they're using (see PEP 517) if they don't want to use distutils.

(We did consider standardizing setup.cfg of course, but the cfg format is pretty bad, it's specifically supposed to be the config file for setup.py which we're trying to deprecate, and then other projects have also jammed stuff in there in an ad hoc way. Migration is kind of up to individual projects, since there's never been any standard for using setup.cfg so who knows what different projects have done. For a project like pbr that's deeply tied to distutils/setuptools, continuing to use setup.cfg probably makes most sense. For a project like pytest that just use setup.cfg as one of the like 4 places they check for config data, migration probably just means adding pyproject.toml to the list of places they check, and then maybe starting a deprecation cycle for setup.cfg-based config, if they feel like it. And for a project like black, you can ignore all this nonsense altogether and pretend setup.cfg never existed :-).)

@ambv
Copy link
Collaborator

ambv commented Mar 30, 2018

I agree with everything you're saying but this really belongs in the PEP and not a comment on a random issue on a random unrelated project on GitHub. cc @ncoghlan

I mostly feel bad for what people like @sigmavirus24 (with Flake8) will have to do to support 3+ distinct configuration files now, having to codify precedence rules and the like. And then, if I take your advice and ignore existence of setup.cfg to use pyproject.toml instead, unsuspecting users will keep pestering me for setup.cfg support for years to come. I don't control their migration schedule. And since Python doesn't have a built-in TOML parser, at this point having your own config file actually seems preferable. We already litter our repos with at least five separate config files, a sixth doesn't make the situation significantly worse. In fact, I always found it confusing why people would prefer to shove all the configs in a single file.

This is why I am staying away from config files for as long as it will be feasible.

@sigmavirus24
Copy link

@ambv yeah, config file support in Flake8 has been a nightmare. My main advice is to support 1 file, that is only ever project local and be done with it. That said, once you've give folks that cookie, they'll want you to support sub-project settings which I'm working towards in Flake8 but I'm not happy about.

And no one has asked flake8 for pyproject.toml support yet. Also, I'm not entirely inclined to add support for that until community adoption is higher than what it seems to be today (my intuition is that it's below 5%).

@njsmith
Copy link

njsmith commented Mar 30, 2018

No tool has to keep their config in pyproject.toml. All in saying is that if you're a project that, today, is considering whether to start putting config into setup.cfg, then you should instead think about whether to put it in pyproject.toml.

I'm not entirely inclined to add support for that until community adoption is higher than what it seems to be today (my intuition is that it's below 5%).

I'm sure it's well below that; we're just at the very beginning of the adoption curve. I think right now the only project that uses it is towncrier? But the next pip release (due within the next few weeks) will start reading build dependencies out if it (like setup_requires but it actually works), and hopefully the one after that will implement PEP 517 and let people start switching away from distutils/setuptools, which I expect will be a pretty good carrot. (I know all my pure-python packages are switching to flit as soon as this happens.)

@ncoghlan
Copy link

ncoghlan commented Apr 1, 2018

From a pyproject.toml perspective, there's no major difference between setup.py and setup.cfg: they're distutils/setuptools specific config files that installers only need to care about for distutils/setuptools based projects (and in that regard, setup.cfg is of even less interest to installer front ends than setup.py, as it's the latter script that actually reads the file - hence why the PEP doesn't even mention the build system specific config file).

So black directly supporting setup.cfg wouldn't make sense, since it would assume that all projects are going to be using setuptools/distutils at a time where we're actively working to give folks more freedom of choice as to which build system they use - it would be akin to natively supporting storing config settings in flit.ini.

For pyproject.toml, the problem is in the other direction: while that may be a good option eventually (since you'll be able to assuming new projects will include it, even if existing ones don't), right now it's still too new to be useful to the vast majority of projects.

As an interim option, something you may want to consider is pointing folks towards the use of tox with skipsdist=True, as that allows particular command line flags to be captured as part of the environment definition, and there's plenty of precedent for tox -e flake or tox -e pylint as a way to run code analysers.

@njsmith
Copy link

njsmith commented Apr 1, 2018

For pyproject.toml, the problem is in the other direction: while that may be a good option eventually (since you'll be able to assuming new projects will include it, even if existing ones don't), right now it's still too new to be useful to the vast majority of projects.

Eh, if you're going to ask people to add a new config file, they can create a pyproject.toml just as easily as they can add a black.cfg. Towncrier uses pyproject.toml exclusively, and people seem to be OK with creating it for that:

@ambv
Copy link
Collaborator

ambv commented Apr 1, 2018

I like the Tox idea because it's an integration framework, like CI or editor plugins. Lets people do exactly what they want while I can continue living in my naive zeroconfig fantasy world.

@sigmavirus24
Copy link

Really any kind of automation makes that easy, tox, Makefiles, fabfile, pyinvoke, etc. There are lots of options and they're as flexible (and all can be checked-in to source control) as a config file as well as future proof.

@emonty
Copy link

emonty commented Apr 4, 2018

I wanted this feature but just went with adding a 'format' env to tox.ini with "black -l 79 foo" in it and that seems fine to me. (the arguments for 88 char line-length all make sense, but I have a small screen on my laptop and am frequently hacking in vim default-sized 80x25 xterms, so code with line lengths above the stanard pep8 length are difficult for me to deal with)

The main reason I wanted it is so that it would be easy for devs with black installed to run black in the repo and have it do the right thing. The tox env provides that just as easily (and lets me say "basepython=python3.6"), so between that and the setup.cfg/pyproject.toml situation, I think the tox approach is gonna do it for me.

@ambv
Copy link
Collaborator

ambv commented Jun 1, 2018

Given additional functionality in 18.6b0, I'm looking at pyproject.toml support. As @sigmavirus24 is pointing out, this is a trap. I just found this fun example where everything is terrible:

a
├── b
│   └── c
│       ├── d
│       │   └── file.py
│       └── pyproject.toml
└── pyproject.toml

When the user runs black a, they would expect that a/b/c/d/file.py is going to be formatted according to a/b/c/pyproject.toml, not a/pyproject.toml. So a single run of Black would have to support potentially many configurations.

Better yet though: a/pyproject.toml might have excluded (see #270) a/b/c entirely. With naive config shadowing, that would not work. Also, we can't just compose previous includes and excludes because they can easily be contradictory. They are freeform regular expressions after all.

So unless somebody comes up with a brilliant solution for this, my current inclination is to support one configuration. That configuration would be searched for in the common base directory for all paths passed to black and this directory's ancestors. A presence of a pyproject.toml deeper in the tree would raise a warning on stderr saying "warning: ignored a/b/c/d/pyproject.toml, configuration in a/pyproject.toml used instead."

While this won't enable people to nicely specify sub-directories which are, say, Python 3.6+ safe or use a custom line length (for tests/ or whatever), they can set up an exclude list in the base configuration for those directories, and configure them separately. That will require running black separately on those directories but I think that's a fair limitation.

@ambv
Copy link
Collaborator

ambv commented Jun 1, 2018

tox -e format still looks preferable to all this :( That being said, I don't want to end up forcing all editor integrations to have to support their own configuration, esp. that people will ask them for project-specific configuration sooner or later.

@sigmavirus24
Copy link

So I do wish I had considered this in Flake8 a lot sooner. In essence, I think what I plan to do is (when I ever have the time) make it such that one config file can specify paths that have their own rules. This will simplify how we find/understand config but also allow the granularity that people want. In Flake8's case, we have an abstraction called the "StyleGuide" so I was thinking of making one extra layer that manages a set of StyleGuides based on the config file. I'm not sure that helps at all, but that's my 2 pence

@ambv
Copy link
Collaborator

ambv commented Jun 4, 2018

I agonized over this for some time over the weekend. I really don't want Black-specific configuration in the CLI as it creates a number of important issues:

  • it weakens the gravity of the defaults;
  • it opens the door for future requests to introduce even more configurability, in other words it weakens the argument from the README that the code style is not configurable;
  • it creates an implementation problem around nicely merging file config with command-line option overrides (not something that is natively supported by Click);
  • pyproject.toml is supposed to be a one stop configuration for all tools in Python projects, which may cause future configurations to grow in size, slowing down parsing;
  • any config file would need extensive documentation;
  • any config file would introduce the "composable overrides" problem I outlined above.

I will hold off with this. For now I recommend sticking to the defaults which makes life easy. If custom integrations are necessary, I recommend one of:

$ tox -e format
$ pre-commit run black [--all-files]

The latter is already described in the documentation under "Version Control Integration".

@dstufft
Copy link

dstufft commented Jun 6, 2018

FWIW, I think that this is pretty important feature for black. Particularly given that --exclude isn't additive, but replaces the default excludes, meaning that for our (pip) use case, the command to execute ends up looking like black --exclude /(\.git|\.mypy_cache|\.tox|build|dist|src/pip/_vendor)/ ..

I think within a singular project you're going to end up with people running black in many different locations, for pip I know that if we adopt black, there is a desire to have the following workflows work:

  • People who use black formatting via their editor.
  • People who have tox installed, and are happy to just run tox -e format.
  • People who don't have tox installed, but are happy to install black to run black.

We might potentially also add

  • A Pre commit hook that contributors can install.

So that's 3, maybe 4 mechanisms of invocation, of which 2 of them would require basically just documenting a regex that uses have to copy/paste into their shell or another config file (and then convince them to update it if we ever need to adjust that regex). That's not a great situation to be in.

In response to the problems listed in #65 (comment), i'd say:

  • People who are using the (small amount of) configuration that black enables, are presumably doing so for good reason. Allowing them to more easily use that configuration that they do have means that projects that need that configuration are more likely going to be able to use black. Omitting the ability to do this just feels like it makes the experience of using black significantly worse (imagine a new contributor coming into a project seeing it uses black, and running just black on it, ignoring the configuration that the project expects and then getting linting errors and being told to go back and redo it. That's friction that makes the contributor more likely to just drop off and not re-contribute).
    • This also requires things like editors to gain a per-project configuration for black, since one project might use --line-length 88, while another uses 80, and another uses 120.
  • I don't think this is true, I think having --line-length at all opens the door for that (and the similar options like --py36 and --skip-string-normalization). I think making it practical to use the configuration that you already have doesn't do anything more to open that door.
  • I don't think merging is an issue, at least not with the settings you have now. None of your settings use append or merging now , they're all just flags or value overrides. So you don't really need much beyond like taking the options from the config file, shoving them in a dict, and then taking the options from the CLI and calling .update() on it. You don't have any data structures that need a deep merge.
  • I think you should use pyproject.toml, and I think the concerns about slowing down parsing are.. premature. Walking the filesystem and reading and parsing all of the .py files is likely going to take a lot more time than parsing TOML. I found some random huge JSON file and converted it to a TOML file, that TOML file was 171MB large and in a completely unscientific test of python -c "import toml; f = open('citylots.toml'); d = toml.loads(f.read())", that took 2m30s to read the file and parse it, which gives us like, 1.2MB/s. I really don't think we're ever going to get a pyproject.toml file that hits anywhere close to even a full MB in size (and if some project out there does, that project is going to be an extreme edge case). I'm going to guess that the random toml library I pulled off of PyPI is also not optimized to it's fullest so if performance. However, if you're really concerned about this, use a different file, I think that's less optimal but still fine.
  • You don't have much in the way of configuration, so your documentation isn't going to be very long anyways.
  • I'm not sure what this means exactly, but I don't think it's an issue. Like I said above, you don't have any structures where you need to do any sort of deep merging. This should be as simple as a {data from config file}.update({data from CLI}).

That being said, I wouldn't support path based configuration. That seems like a rabbit hole of mess that you really don't want to be in. Generally supporting multiple formats for different files seems like a huge edge case (and goes against PEP8 to begin with, since consistency within a project is the most important constraint).

@sigmavirus24
Copy link

You don't have much in the way of configuration, so your documentation isn't going to be very long anyways.

I wonder what you think of http://flake8.pycqa.org/en/latest/user/options.html @ambv as an example of documenting the CLI and config file options together.

@ambv ambv closed this as completed in 489d00e Jun 7, 2018
@njsmith
Copy link

njsmith commented Jun 7, 2018

The multiline regex handling jumped out at me as a little surprising... Maybe you should accept a list here? (TOML has types!)

@dstufft
Copy link

dstufft commented Jun 7, 2018 via email

@ambv
Copy link
Collaborator

ambv commented Jun 7, 2018

@njsmith, the configuration file literally passes data to the respective options in Click. I'm actually pretty proud of how concise this ended up being. The --exclude= command-line option accepts a regular expression.

The multiline regex enables us to do rather fancy filtering if needed without me having to implement fancy globbing.

@njsmith
Copy link

njsmith commented Jun 7, 2018

That's how re works, but before you get to that point you have to decide whether a regex is the most natural way to specify a list of files :-). (On the command line you could also take multiple --exclude / --include options; that's pretty common.)

konradkonrad added a commit to konradkonrad/raiden that referenced this issue May 6, 2019
The `black` project decided not to support `setup.cfg`, but only
`pyproject.toml` (see psf/black#65).

In order to avoid multiple configuration places for
our `black` defaults, I added this additional configuration file to
the repo.
konradkonrad added a commit to konradkonrad/raiden that referenced this issue May 6, 2019
The `black` project decided not to support `setup.cfg`, but only
`pyproject.toml` (see psf/black#65).

In order to avoid multiple configuration places for
our `black` defaults, I added this additional configuration file to
the repo.
rakanalh pushed a commit to raiden-network/raiden that referenced this issue May 6, 2019
The `black` project decided not to support `setup.cfg`, but only
`pyproject.toml` (see psf/black#65).

In order to avoid multiple configuration places for
our `black` defaults, I added this additional configuration file to
the repo.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: style What do we want Blackened code to look like?
Projects
None yet
Development

No branches or pull requests

8 participants