Format all of Spack with black

[tgamblin]

This blackens the spack repository, scientifically 😄.

We want:

1. To be able to fix most flake8 errors automatically, so as not to burden people with code style.
2. To keep configuration simple and avoid having two code styles (one for packages and one for core).
3. To avoid forcing a tool on package contributors who aren't all familiar with the python ecosystem. i.e., we do not want to hold up anyone's package PR on style -- we want this to be faster than what we're already doing, even without a tool installed.

How many columns?

Black defaults to 88 columns, but since we want to have one width for both core and packages, that is too narrow. You can see the effect of blackening packages and core for different column widths here:

Using 88 or fewer columns would add about 100k lines to the code base. We don't want that. Given the data and the spirit of black's 88-character limit, we will use a width of 99 characters because:

• 99 is one less than 100, a nice round number, and all lines will fit in a 100-character wide terminal (even when the text editor puts a \ at EOL).
• 99 is just past the knee the file size curve for packages, and it means that packages remain readable and not significantly longer than they are now.
• It doesn't seem to hurt core -- files in core might change length by a few percent but seem like they'll be mostly the same as before -- just a bit more roomy.

See this comment for more details.

How to fix style automatically?

Once this is merged, you can do a few different things to fix Spack's style automatically:

1. Run spack style --fix -- this will automatically bootstrap and run isort, mypy, black, and flake8 in that order, and it will automatically fix isort and black errors. black fixes most flake8 errors, but flake8 still does useful checks (e.g., finds unused variables), so we still run it.
2. Type @spackbot fix style in a PR comment. Spackbot now knows how to run spack style --fix, in case it doesn't work in your environment for some reason.
3. Use black yourself. Note that you will need to use black at version 21 or older, as we still support Python 2.7. Note that black requires Python 3, but versions 21 and before will check 2.7 code. Versions 22 on drop support for 2.7.

What else is different?

Because black formats them automatically, we no longer allow exceptions for long lines in Spack, except those that contain URLs. The only way you can get a really long line now is if you have a really long string, and you will need to either:

1. Break up multi-line strings with parentheses, e.g.:

   depends_on(
       "boost@:1.63.0"
       "+chrono +date_time +filesystem +iostreams +mpi +numpy +program_options"
       "+python +regex +serialization +system +test +thread +timer"
   )

   or:

   long_string = (
       "llvm"
       " targets=amdgpu,bpf,nvptx,webassembly"
       " version_suffix=jl +link_llvm_dylib ~internal_unwind"
   )

   Strings next to each other like this are automatically combined into one by Python (like C).

2. Use multi-line strings, e.g.:

   this_is_a_long_string = """\
       here is some text
       and here is some more
   """

Ignoring the huge reformat in your history

Run this in your Spack repo so that you won't see the gigantic black reformat commit when you use git blame:

git config blame.ignoreRevsFile .git-blame-ignore-revs

Details

• Set line length to 99.
• Remove most exceptions from .flake8 and add the ones black cares about.
• Add [tool.black] to pyproject.toml.
• Make black run if available in spack style --fix.
• Format everything with black.
• Add .git-blame-ignore-revs file so reformatting does not pollute GitHub blame.
• Make spack blame aware of .git-blame-ignore-revs.
• Add constraints so that spack will always bootstrap black@:21.
• Break up long strings that black can't handle.
• Make spack create and spack checksum black-compliant.
• Add a code style badge to README.md.
• Remove --no-black, --no-isort, --no-flake8, --no-mypy arguments to spack style.
• Add --tool TOOL and --skip TOOL arguments to spack style so we can be specific.

[trws]

Oddly I can't repro the spack.compilers failure on a local machine, it's like the path is getting messed up or something. The config looks pretty good, but something strange is happening with imports. My first guess is that somehow having paths first is accidentally important, which is slightly scary...

[tgamblin]

Ok trying again now that #24695 is in. I think we need to allow packages to ignore black for long directives with SHA's. I think that includes patch, version, and nested patch directives, or maybe it's just version. I think the black-formatted version lists are much harder to read/scroll through than they were before.

@adamjstewart: thoughts?

[adamjstewart]

Let me download locally and see how badly black has messed things up.

[adamjstewart]

Okay, I took a look at a few big packages that I maintain. I'm actually surprisingly okay with most of the changes? I agree that version lists are a bit more verbose now than they used to be. I wouldn't be opposed to some solution that causes black to leave them alone (or formats them consistently and allows them to be longer).

Black doesn't allow for much configuration, right? Would spack style simply comment those lines out before running black and uncomment them afterwards? Or do comments also get wrapped? I know we have a lot of exemptions in flake8_formatter.py that allow us to ignore things, but I don't think they'll work for black. # fmt: off only works at the file level.

[tgamblin]

I think inserting # fmt: off and # fmt: on is complicated. My current thought is to do a pre-pass that removes sha256's and replaces them with short ids, formats everything, then puts them back. So essentially you'd get the formatting for a 0-ish-length string for strings that are "too long to format". I could probably do the same for URLs. The advantage of this is that it's a simple rule and easy to parse.

[tgamblin]

That, or maybe I could get used to this. OpenMPI, HDF5, and LLVM are ones with long version lists, and they're not awful. I looked at MFEM and my heuristic above doesn't work very well for it -- if I substituted both URLs and SHA's out of the version() directives there, we'd have annoyingly long version() lines. There probably isn't a great answer and I'm probably overthinking this and forgetting that the whole point is that this does auto formatting so I don't have to care.

[adamjstewart]

Let's get input from a few more people. I think the main reason we originally exempted all of Spack's directives from flake8 was because of me. I think I could also live with things as they are, especially if spack style --fix does it all for me. I would also be fine with replacing all URLs/checksums with a placeholder as a pre-pass, even if that means that some version directives will end up as very long lines.

[tgamblin]

The other possibly much simpler option would be to go to a line length of 100. I tried it and it only adds 7k lines to Spack, vs. 60k for a line length of 88. It formats most directives nicely -- notably variant() and version(). Some of the lines (mostly variants) that looked kind of bad before are actually easier to read now.

[adamjstewart]

I would also be okay with that solution.

[tgamblin]

I kind of like it better because it's uniform across the code and the packages, and we do not have to make exceptions for our package DSL -- something that's been a bit awkward when looking at package files for a while.

[trws]

I think inserting # fmt: off and # fmt: on is complicated. My current thought is to do a pre-pass that removes sha256's and replaces them with short ids, formats everything, then puts them back. So essentially you'd get the formatting for a 0-ish-length string for strings that are "too long to format". I could probably do the same for URLs. The advantage of this is that it's a simple rule and easy to parse.

Could we consider offering a "versions" directive to take a large number of fees ions and hashes instead and just let maintainers use it if they care? Or increase the allowed width for packages to account for hashes? This is a lot of complexity for relatively little IMO.

[trws]

Having read later comments I'm also good with a length of 100 for packages. The 88 length in black is because the author found on codes they tested on that it was best for reducing spurious extra lines like that. If 100 is more appropriate for packages in spack we should do it.

Thinking about it having a multi-version directive might still be nice though, might mock that up and see how it looks.

[tgamblin]

I'm running the numbers on this as we speak. Leaning towards 99 (so that editors can put the \ in a 100-wide terminal). Will post plots when done. Basically I guess we should do what for spack what the black maintainers did for black.

[tgamblin]

Ok here are the relevant numbers. I blackened Spack for line lengths from 79 to 120, and I looked at the total lines in python files for each of those settings.

Horizontal lines are current total lines in core, packages, and both. Vertical lines are the settings we're considering.

Core

There isn't much of a difference for core -- there isn't really a knee in the curve and the benefits are small as the limit gets longer.

Packages

For packages, there is a big knee in the curve around 96 characters -- that's the length of a line like this:

    version("x.y.z", sha256="5f9a3ee85db4ea1d3b1fa9159352aebc2af72732fc2f58c96a3f0768dba0e9aa")

There are a lot of those in packages. There is a smaller knee at ~90 characters, which is where I think most variant() directives can fit on a line.

If we choose 88 as our limit, the packages will be around 40% longer than they are now. I think that's bad for readability, and we already felt strongly enough about versions, URLs, etc. fitting on a line that we made exceptions for directives in the original spack flake8 command. I think contributors and package maintainers will not like to see lots of empty lines between their versions.

Recommendation

Given the data and the spirit of black's 88-character limit, my inclination is to set the limit to 99 characters for all of Spack, because:

• 99 is one less than 100, a nice round number, and all lines will fit in a 100-character wide terminal (even when the text editor puts a \ or other character at EOL).
• 99 is just past the knee in our curve, and it means that packages remain readable and not significantly longer than they are now.
• It doesn't seem to hurt core -- files in core might change length by a few percent but seem like they'll be mostly the same as before -- just a bit more roomy.

The only downside I can see is that 99 is a bit longer than our longstanding limit of 80, and we might not be able to fit everything in an 80-char terminal anymore. I think 100 is still on the smaller side, so you can have a few terminals open, and they will fit on a screen.

Thoughts?

[becker33]

@tgamblin that looks good to me -- I think 100 char terminals won't offend many people.

[alalazo]

@tgamblin I also think that's a good choice. I'd be fine with anything in the 100-120 chars range

[alalazo]

I know I am stating the obvious, but we need to advertize this PR before we merge it since it has the potential to conflict with everything.

[trws]

As I mentioned before, I’m in favor, but do take a look at the output in core code before making the final decision. Since black will put things entirely on one line when it can, a long line limit there may make certain things, dict literals particularly, a bit rougher. Probably still worth it in the balance to keep things consistent, but worth a look to verify.

[adamjstewart]

💯 works for me!

[adamjstewart]

Contains everything I was looking for. Can't check individual files, but once the tests pass, this should be good to merge!

[adamjstewart]

Did you also update https://github.com/spack/spack/blob/v0.18.0/lib/spack/spack/build_systems/autotools.py#L327? Not sure if there are any other places in Spack where we print package.py content.

[adamjstewart]

Oh, all of the docs still probably use the old format. Not crucial, but still worth updating someday (not necessarily in this PR). Does anyone know of a Sphinx plugin that lets you run black on .rst files?

[tgamblin]

I didn't but I don't really think it's worth updating that in this PR. Diff coverage is wrong -- it's 92% per codecov (if you click the link). I dunno why that number becomes more wrong the longer the PR has been around.

[tgamblin]

This PR is tricky b/c it has a commit ref in it (in the blame ignore file), so it really needs to be rebased and fast-forward on develop. Looks like it can go now, but if anything gets merged to develop so that it's not up to date we'll need to update that file after the fact.

[vsoch]

Totally missed this earlier! Just wanted to say yay! 🥳