Add APE to adopt Black formatting in astropy

[taldcroft]

This APE proposes that the astropy core package adopt Black as the default code format.

Following discussion of this topic on astropy-dev, @eteq and @taldcroft developed this together over a series of virtual meetings, sharing our different perspectives and working to converge to a common view. We are hoping this draft APE will generate constructive discussion and hopefully lead to adoption of the APE.

[kelle]

non-substantive edits. LGTM.

[WilliamJamieson]

Adding pre-commit documentation is already in progress as part of astropy/astropy#13552.

We already provide a pre-commit configuration and use it for our current "Code style checks" already, it just has not been properly documented in the astropy documentation.

[WilliamJamieson]

astropy/astropy#13552 has been merged, updating the documentation about how to use pre-commit is no longer necessary.

[WilliamJamieson]

The usage of the pre-commit.ci bot proposed in astropy/astropy#13320, see the implementation plan astropy/astropy#13320 (comment) will perform all of these tasks for us. The function of this bot will be documented by astropy/astropy#13553 when we enable the bot.

Note the usage of pre-commit and the related pre-commit.ci bot are separate issues than adopting black.

[WilliamJamieson]

The pre-commit.ci bot was turned on today and documented with the merger of astropy/astropy#13353, so in the event that this APE is accepted, simply adding Black (or some other formatting tools) to the pre-commit configuration will be sufficient to enable opt-in automatic fixing.

[WilliamJamieson]

PR astropy/astropy#13253 sets up the changes to astropy to enable enforcing Black on a per-subpackage basis, but does not run black on any part of astropy. Note that the changes to look at are to the .pre-commit-config.yaml and pyproject.toml files, all the other file changes are due to a necessary change to the configuration for isort so that it is compatible with Black.

One minor change to this PR, so that it fits the currently proposed APE, is updating the line_length values (for flake8, isort, and Black) so that they all conform to the default Black line length of 88.

[nstarman]

Or change Black to a line length of 100 (https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html?highlight=length#line-length)

[taldcroft]

Default (88) please! 😄 I've been using Black with 88 and I like it.

[Cadair]

I heard we were painting bike sheds. Longer lines ftw.

[astrofrog]

Split the difference, and go with 94?

[nstarman]

I'm good with any. Personally, I prefer 100+, but so long as it's consistent...

[adrn]

I'm used to the default 88 but I don't feel very strongly.

(When I first read this APE, I was still pro-yapf because of the customizable features (esp. around array reformatting), but now that I see how much discussion we would have around each setting, I'm glad black only has a few 😅.)

[eerovaher]

88 is plenty, I see no reason not to use the default value.

[astrofrog]

I don't really mind either way, my suggestion of 94 above wasn't necessarily very serious.

[taldcroft]

OK, I'll revert back to 88. I don't mind, I just want something. 😄 But personally I like the simplicity of using the default.

[WilliamJamieson]

Note that string normalization converts the quotes around all strings to be " unless the string in question contains a " character.

[WilliamJamieson]

An add-on comment, should we consider applying isort to astropy while performing the all the Black changes. Tagging @nstarman to explain isort in more detail.

[nstarman]

If it's not too burdensome for this APE, bundling black + isort might be worthwhile. They are are frequently used together and we already have a similar per-package setup for isort...

[nstarman]

Some sentence fragments here.

[taldcroft]

Will fix.

[astrofrog]

I'm generally on board with this but I would like to see more details on how/when we will do this logistically to minimize impact.

[astrofrog]

I'm not sure that touching every file is really that bad, the concern is more here that it might in fact touch most lines (which is still fine because we can mark the commit to be skipped, but I do think this is unprecedented). If you keep the last sentence, give some examples?

[taldcroft]

Indeed this touches about a 1/3 of lines in the time subpackage (see below). So I'm happy to rewrite this paragraph as:

This changes almost every Python file and a large fraction of code lines 
(for example about one third of non-blank lines in the ``astropy.time`` 
subpackage). This is more than previous bulk changes and it is an impact that
needs to be accepted with this APE.

[astrofrog]

Wrap text and spell out and explain what AST is.

[Cadair]

One line per sentence is the correct way 😉

[bsipocz]

you mean one line per word 😉

[taldcroft]

Will do.

[astrofrog]

Is it necessary to have the paragraph break with the :?

[taldcroft]

Will fix.

[astrofrog]

This section is missing something about how we will deal with release branches, and when the change will be done, both to minimize impact on release branches and open PRs. As far as I can see there are two options:

• Make the change just before the first 6.0 release candidate is made, because at that point the release branches will no longer be active and open PRs may be at a minimum.
• Have a sprint some other time to minimize open PRs, then pick an arbitrary date to make the change and simultaneously reformat main and any active release branch.

[taldcroft]

I like the second option.

The main concern there is the extent of exceptions that are put in by hand for main. The APE is explicit in allowing exceptions, but I don't see an easy way to apply those exceptions to formatting any active release branch -- they would be formatted with a single run of black on the entire repo. I'm hoping that in practice the exceptions will be rare and won't generate conflicts.

[WilliamJamieson]

I agree as well. Indeed we may want to plan a sprint to coincide with the 6.0 release so that we can push all these changes in quickly. The creation of the initial Black changes will be simple, but reviewing the changes for any possible exceptions will be the tedious part, and we may want to have people ready to do this work.

I worry about any remaining open PRs after Black changes are merged. We should work with the contributors of any of the remaining open PRs to both warn them and help them get their PRs migrated.

[taldcroft]

I was wondering about a hybrid of the options --

After the normal v5.2 feature freeze on 2022-10-28, then have a 2-week period for doing the Black transition of all sub-packages. I think the initial PR's could be done by a core team of a few, but this requires some buy-in from sub-package maintainers to do the reviews. By doing this in a special post-freeze period this might minimize the impact on open PR's. Many PR's that stay open through a feature freeze are not being actively worked.

The 6.0 freeze is not until 2023-10-27 so I hope we don't need to wait that long!

[nstarman]

I like the sound of this (esp the earlier deadline 😆 )

[taldcroft]

Release managers @astrofrog @bsipocz @saimn @embray, and others - are you OK with the plan of a 2-week post-freeze period to do the Black transition PRs for sub-packages? Or does anyone have a better plan?

[eerovaher]

darken can apply black formatting to only the lines that are being edited. This would allow for a transition period during which all new code would be black-formatted, so when the final switch to black finally happens the diffs would be smaller.

[astrofrog]

I'm happy with a 2-week post-freeze period, though we will need to decide what to do for sub-packages where maintainers end up being unavailable to review.

I think we might want to not run black on the LTS branch but as that branch has existed for almost a year the number of bug fixes that backport cleanly is going to be more and more limited, so maybe we just don't format the LTS branch and deal with conflicts by hand.

[nstarman]

Personally, I've found darken more finicky than black to get working with IDEs. Not saying I'm against it, but going all in would be my preference.

[astrofrog]

Not clear what 'compatible' means - maybe better said as 'This APE would superseded any previous code formatting styles recommended for the core package'

[bsipocz]

the enforcing of pre-commit bot, without black could be compatible, and therefore I feel it's a mistake to merge these two proposals together and sell the advantages of one as the advantage of the other and a reasoning to live with the disadvantages. (e.g. pre-commit without black but enforcing linting has the huge potential to be the middle ground)

[WilliamJamieson]

As far as pre-commit and the pre-commit bot are concerned, the infrastructure team has been moving forward with using them in astropy independent of this APE. The introduction of pre-commit has already occurred, while this APE was being drafted it was realized that pre-commit was not documented for astropy developers. The use of the bot is a natural extension of using pre-commit which should help ease the burden of any current (or future) linting or style checks on contributions.

[taldcroft]

I'm generally on board with this but I would like to see more details on how/when we will do this logistically to minimize impact.

This is where I'm hoping that folks in the infrastructure team can help out. Personally I don't know enough of the details in that area to make a detailed impact statement.

One thing I can say is that it is correct that a large fraction of lines will be touched. Applying black to astropy/time resulted in 22 files changed, 3599 insertions(+), 2599 deletions(-) in about 10000 lines of code (non-blank lines).

[taldcroft]

An add-on comment, should we consider applying isort to astropy while performing the all the Black changes. Tagging @nstarman to explain isort in more detail.

I've gone back and forth, but it seems like keeping them separate might be easier. Since isort is less controversial, I could see doing that for the entire astropy package in one commit. I think that would be easier and decouple this from the package-by-package process for black.

[mwcraig]

LGTM, just one or two minor comments.

[mwcraig]

Not a big deal, but seems like this should be a bulleted list.

[bsipocz]

I feel bringing Django into the argument is comparing apples to oranges. black is absolutely awful with mathematical expression formatting as well as with nested containers, so comparing the astropy case to e.g. scipy would be more realistic.

[taldcroft]

Django is similar to astropy in being a large community-driven open-source project with a range of experience levels for contributors. So the comparison is useful in terms of developer and community experience.

As mentioned elsewhere, formatting of the code and the small fraction of cases where this goes badly is not a problem when you actually run black on astropy and look at the results. For instance try modeling, which is probably one of the more math-heavy parts of astropy. A lot of equations are changed, and IMO the changes are mostly improvements and at worst neutral.

But again, if the developer and PR reviewer both agree that black is doing badly then the that code can be skipped.

[bsipocz]

consistent and readable code formatting can be achieved without black. However there has been a pushback from the community for year to enforce linting, even though that step could be done along with pre-commit hooks, and without black. Forcing the pre-commit hook to be run would similarly result of fixed code without separate pep8 commits, so it is kind of an unfair accounting to claim that black would fix this issue.

[bsipocz]

for completeness I would also suggest to cite the projects that decide of not to go down this route. E.g. quite a couple of projects are waiting for black to implement some reasonable approach for mathematical and nested structure formatting.

For an absolutely ugly reformatting example, one can look at scipy: scipy/scipy#14330 where black wouldn't result a more readable code, but a bloat of 45% of number of lines.

[taldcroft]

@bsipocz - good idea, can you provide a list of other projects that have decided to either (1) reject auto-formatting entirely as being a bad idea or (2) wait until the black implementation is better.

The APE states that cases where black does poorly can and should be excepted. Based on testing by myself for time/table/io.ascii and @eteq (coordinates), requiring exceptions is not common but they will be allowed. Any others e.g. @WilliamJamieson @nstarman have data on the need for exceptions?

[bsipocz]

a few projects (scipy is rejected, numpy is waiting, they may be others, too) are waiting for the math formatting summit, and the promise from the side of the black developers to take scientific python needs into account. Most of those mailing list and discourse and PR discussions are linked to the scipy and numpy PRs cited above.

afaik all that implemented it and likes it, in fact likes the fact that linting is now enforced, rather than the formatting itself, and thus my opinion of enforcing linting would do the job without enforcing to live together with the nonsense of the choices black forces on us (e.g. all of those in the project were against linting cited less serious issues and have been unwilling to accept the choices of other team members, but now are willing to accept the nonesences around the 3rd party, black's verbose whitespacing?)

[taldcroft]

@bsipocz -I don't know precisely what you mean by "enforcing linting". We already do CI flake8 checks, so do you mean something different?

What I like most about black is fast auto-formatting in my IDE. But in general I am happy with the format choices black makes, so you should not presume to know the opinions of the large community of black users.

[bsipocz]

currently, a select number of linting options are checked, rather than only a few ignored, and that is historical reasons as there was a push against being strict on linting, being conservative and allowing interpretation of the pep8 rules as basically, the aim was to keep the heterogeneity of the different submodules. We wanted and could have done e.g. autopep8 to auto format those things that are uncontroversial.
So now, the proposal is to be being instead extra strict and enforcing linting choices that are blindly doing PEP8 and not necessarily ideal for the codebase we have.

And I don't claim to know the opinion of all black users, but say that there is a well documented shared concern in the wider scientific python community that its enforced code style is not ideal for math and array-heavy usage. Beyond this shared concerns I have personal issues with the superfluous white spaces it applies, the 30-45% addition of lines with no useful information (single parens or characters). I also have personal preferences for IDE and editor behaviour which I'm very happy with, but am not trying to force that on all the other developers.

[nstarman]

The APE states that cases where black does poorly can and should be excepted. Based on testing by myself for time/table/io.ascii and @eteq (coordinates), requiring exceptions is not common but they will be allowed. Any others e.g. @WilliamJamieson @nstarman have data on the need for exceptions?

In my experience, I haven't found too many places where exceptions are needed.
Sometimes / often a blackened math one-liner looks worse because of the spaces around operators. However, I tend to have comments by math code with a cleaner representation of the math, because except for very simple expressions, I don't find math in Python very readable, regardless of style. For multi-line complex math, my thoughts on Python and needing explanatory comments are even more true. IMO Black doesn't usually make the end result worse as it encourages me to rewrite the math.

tl;dr, piles of multi-line math are much less readable for future maintainers than well factored code with comments. Sometimes multi-line math is unavoidable, which is why we can selectively turn black off.

[bsipocz]

An even more undesirable formatting (though not with this small, int array, but a large float one would be the examples present in the scipy PR: scipy/scipy#14330)

self.a = asarray([1.5750000E-04, 1.6990000E-04, 2.3500000E-04,
                          3.1020000E-04, 4.9170000E-04, 8.7100000E-04,
                          1.7418000E-03, 4.6400000E-03, 6.5895000E-03,
                          9.7302000E-03, 1.4900200E-02, 2.3731000E-02,
                          4.0168300E-02, 7.1255900E-02, 1.2644580E-01,
                          2.0734130E-01, 2.9023660E-01, 3.4456230E-01,
                          3.6980490E-01, 3.6685340E-01, 3.1067270E-01,
                          2.0781540E-01, 1.1643540E-01, 6.1676400E-02,
                          3.3720000E-02, 1.9402300E-02, 1.1783100E-02,
                          7.4357000E-03, 2.2732000E-03, 8.8000000E-04,
                          4.5790000E-04, 2.3450000E-04, 1.5860000E-04,
                          1.1430000E-04, 7.1000000E-05])

vs

        self.a = asarray(
            [
                1.5750000e-04,
                1.6990000e-04,
                2.3500000e-04,
                3.1020000e-04,
                4.9170000e-04,
                8.7100000e-04,
                1.7418000e-03,
                4.6400000e-03,
                6.5895000e-03,
                9.7302000e-03,
                1.4900200e-02,
                2.3731000e-02,
                4.0168300e-02,
                7.1255900e-02,
                1.2644580e-01,
                2.0734130e-01,
                2.9023660e-01,
                3.4456230e-01,
                3.6980490e-01,
                3.6685340e-01,
                3.1067270e-01,
                2.0781540e-01,
                1.1643540e-01,
                6.1676400e-02,
                3.3720000e-02,
                1.9402300e-02,
                1.1783100e-02,
                7.4357000e-03,
                2.2732000e-03,
                8.8000000e-04,
                4.5790000e-04,
                2.3450000e-04,
                1.5860000e-04,
                1.1430000e-04,
                7.1000000e-05,
            ]
        )

[taldcroft]

Since this example is not even in astropy, and we would just skip black formatting anyway, I'm not sure what this is telling us.

[bsipocz]

and we would just skip black formatting anyway

Then what this APE is about, did I miss the suggestion of not accepting the APE? IMO this is a very much representative example of what's wrong with black, more so than the example in the APE. And there are arrays in astropy, much more than in some other packages that are cited as examples what opted in using it.

[nstarman]

Is this an example from the test suite?

[nstarman]

I'm certainly guilty of this, but we should probably minimize hardcoding in arrays for most tests, and instead use more elegant tools like https://github.com/astropy/pytest-arraydiff

[taldcroft]

@nstarman - the example (or very similar) in the APE does occur in astropy (https://cs.github.com/astropy/astropy?q=%22%5B1%2C+0%2C+0%5D%22) but the long one in the comment here is from scipy.

I agree that Black formatting sometimes highlights that the original code could be improved, which is a good thing. We saw that in the original email discussion with an expression that had around 10 layers of nested parens -- it turned out that it would be possible to rewrite that in a much cleaner way.

[bsipocz]

This is an example from the scipy PR. IMO you cannot ignore mentioning in the APE that neither scipy nor numpy opted in black, exactly because of math and array issues like e.g. this one.
The scipy PR also shows the bloating up the codebase with whitespaces.

And they are both very large and diverse contributor base, and much more similar to the one astropy is having, consisting of both scientists and engineers, as opposed to e.g. django.

[hamogu]

I agree that the numpy and scipy PRs and associated discussion should be mentioned. That does not automatically mean that we need to follow their lead, but mentioning only Django looks like cherry-picking.

[taldcroft]

@bsipocz @hamogu - I added 884b0a1. Is that sufficient?

[hamogu]

Sounds good to me. Thanks!

[bsipocz]

the enforcing of pre-commit bot, without black could be compatible, and therefore I feel it's a mistake to merge these two proposals together and sell the advantages of one as the advantage of the other and a reasoning to live with the disadvantages. (e.g. pre-commit without black but enforcing linting has the huge potential to be the middle ground)

[taldcroft]

I think I have addressed all the review comments requesting a change in the APE text.

[taldcroft]

About the review comments, @bsipocz is fundamentally and strongly against acceptance of this APE, so her dissent is noted here in the APE review but I did not change any of the APE to reflect her views.

[Cadair]

In the name of science: astropy/reproject#308

[taldcroft]

@Cadair - nice example. I made a suggestion on splitting the difference https://github.com/astropy/reproject/pull/308/files/233ccd41bd06fd91f0c856ae1cc6c8890ff71721#r966983968.

[Cadair]

It does provide checks that black doesn't, such as checking that things in the __all__ are defined in the code etc.

[WilliamJamieson]

It does provide checks that black doesn't.

I agree with this, I would go farther and propose that we attempt to enforce as much of flake8 as possible.

[taldcroft]

Fixed.

[Cadair]

fwiw this is already done: https://docs.astropy.org/en/latest/development/workflow/maintainer_workflow.html#fixing-coding-style-issues

[taldcroft]

Fixed.

[Cadair]

Aside from technical considerations and details, I am still decidedly neutral on this.

I think that black is ultimately lazy, but given that in my opinion a lot of things in astropy are not lazy enough, this probably as good of a place to start as any.

[taldcroft]

@Cadair - thanks for the review and inputs. I think I incorporated your points and am definitely 👍 on not being overly prescriptive here.

@WilliamJamieson - thanks for continued helpful inputs.

[eerovaher]

#76 (comment) shows that some kind of autoformatting is required (and with --grep="white\s*space" --grep="[wW]hite\s*space" added the number of matches is currently 563 600). Although black is not perfect, the support from PSF, adoption by many projects and lack of configuration options means that it can be considered standardized, and standardized is second best after perfect. The exact process of adopting black in astropy might benefit from further discussion, but if the question of whether or not we should adopt black would be put on vote then I would vote in favor of adopting it.

[WilliamJamieson]

black is not a perfect solution to formatting e.g. #76 (comment). However, I think a lot of these issues are surmountable via refactoring the code (minimizing hard coded arrays) or turning off black for limited sections of the code. Moreover, it is well supported by the PSF and seems to have a wide general adoption. In any case, I feel like astropy needs a stronger formatting policy which can be automatically implemented and enforced; black, in my opinion, represents the cleanest way to achieve this. If there is strong disagreement with using black I invite those parties to propose alternative tools which implement automatic formatting and enforcement; I am open to better "solutions" to code formatting in astropy than our current lack of one.

Thus, I support this APE. If its accepted I volunteer to help with the implementation of black in astropy.

[nstarman]

I don't love black for math or hard-coded arrays, but I much prefer having black than not. I use it in all my projects as it's fast and has broad and growing support across the Python ecosystem. I'm very much in favor of this APE. Thanks for the hard work in putting this together.

[embray]

I think a lot of these issues are surmountable via refactoring the code (minimizing hard coded arrays)

The idea of refactoring in order to satisfy a style checker is disturbing to me. Now if were talking static typing that's another thing.

But if you just mean wrt hard-coded data, I think it's better to move that into data files anyways (JSON, csv or even npz or something like that as appropriate), so +1 to that.

[taldcroft]

The idea of refactoring in order to satisfy a style checker is disturbing to me.

Recall this actual code from astropy:

            numer = (((((((((((((((AA[15]*Z + AA[14])*Z + AA[13])*Z + AA[12])*Z + AA[11])*Z +
                               AA[10])*Z + AA[9])*Z + AA[8])*Z + AA[7])*Z + AA[6])*Z +
                          AA[5])*Z + AA[4])*Z+AA[3])*Z + AA[2])*Z + AA[1])*Z + AA[0])

Black expands this out into a very long multiline expression, which may or may not suit your taste. But I think it is not disputed that the original (current) version is not ideal. Discussion in the original astropy-dev thread on black found that there would be a straightforward way to refactor this expression programmatically that made it a lot more understandable.

The takeaway being that cases where black does poorly sometimes suggests looking for a better way. Your suggestion of putting substantial data chunks into a files is another good example.

[WilliamJamieson]

The idea of refactoring in order to satisfy a style checker is disturbing to me. Now if were talking static typing that's another thing.

My comment, was more about the fact that when we apply black it tends to point out area's of our code that are less than ideal, such as what was pointed out here: #76 (comment). So black has a tendency to point out areas that we could generally improve. Indeed, the original conversation in google groups discussed how that example could be improved in several different ways.

But if you just mean wrt hard-coded data, I think it's better to move that into data files anyways (JSON, csv or even npz or something like that as appropriate), so +1 to that.

This is mostly what I was thinking. Especially, when it comes to some of our test cases. (Also, I would add ASDF as a possible method for storing this type of data, but that is my personal bias)

[mwcraig]

Thank you for the productive discussion everyone -- the Coordination Committee has accepted this APE, which I will merge momentarily.

[pllim]

Now that all is said and done, here are the lessons learned brought up in 2022-12-06 telecon. I am documenting it here if we ever use this as the model for the next big refactoring APE.

1. Assign a few overseers who can be present and make timely decisions that are consistent with the accepted APE; i.e., a governance structure for that effort. Without this, PR author was forced to make decisions on the spot on a case-by-case basis, causing the APE application to be inconsistent throughout.
2. APE should have had a "tips and tricks" section; e.g., on how to apply black better with trailing commas.