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.

[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.

[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.

[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.

[astrofrog]

A general question - as this is one of the more controversial APEs that has been re-written, and it is unlikely there will be universal consensus, how is the decision process going to be handled? Will a vote be carried out amongst voting members? core package maintainers? Or will this be left to the CoCo to decide?

[taldcroft]

About process, APE 1 says The final decision on any APE is made by the coordinating committee, though usually a consensus in the development is sufficient (but in unusual cases may be overridden by the coordinating committee).

I had been thinking that anyone that wants to weigh in should put in a review, which could include formally approving the APE text. A final call to astropy-dev with a deadline might help. This will give the CoCo information for their decision.

But ultimately I think the CoCo needs to (1) fully define the process and (2) make a decision.

[Cadair]

In the name of science: https://github.com/astropy/reproject/pull/308

[taldcroft]

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

[taldcroft]

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

@WilliamJamieson - thanks for continued helpful inputs.

[eerovaher]

https://github.com/astropy/astropy-APEs/pull/76#discussion_r961640097 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.

[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: https://github.com/astropy/astropy-APEs/pull/76#issuecomment-1249503215. 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.