bluesky / bluesky-enhancement-proposals

0 stars 4 forks source link

Consider applying Black to *some* projects. #16

Open danielballan opened 5 years ago

danielballan commented 5 years ago

The Black code formatter has been gaining traction in Python. It was recently moved into the PSF GitHub org, and a couple scientific Python projects (dask for example) have applied it to their codebases.

I am still against applying Black to old codebases with a lot of history that would be obscured by a giant reformatting commit, but I am personally becoming more open to applying it to new projects. Black's heuristic for formatting is optimized to reduce the size of diffs, which makes review easier. In my opinion, Black makes readability slightly worse than thoughtfully hand-formatted code, but in exchange one gains some efficiency: no more whitespace commits to satisfy flake8 or review comments about whitespace.

I think this trade-off is most appealing in projects where we have to do a lot of rote maintenance, such as the suitcase-* packages. I am also open to applying it to our fairly new projects without much history yet, like bluesky-browser, bluesky-darkframes, bluesky-spreadsheet, and intake-bluesky.

As a slight aside, the trade-off is also appealing in projects where some frequent contributors aren't fluent in Python style conventions, such as in NSLS-II beamline IPython profiles and beamline-owned utility projects like NSLS-II-CHX/chxtools. This may be the case for analogs at other facilities too.

How would folks feel about applying Black to the suitcase-* repos to start and seeing how we like it?

danielballan commented 5 years ago

This was discussed on last Friday's call. See notes.

There is a strong consensus for using flake8 and flake8-bugbear (which adds some tolerances to flake8). Many of us are already using editor integrations that apply flake8 checks anyway.

There is consensus to try the auto-formatters, black (very opinionated reformatting), isort (sorts import statements), and removestar (changes from __ import * to explicit statements) on our new, small projects and see how we like it. It seems like everyone on the call has some reservations but overall feels this is likely to save time and effort for more important matters.

jklynch commented 5 years ago

I had not heard of Black before. I have spent some time in the past looking for a Python code formatter but I never found 'the one'. I am very happy with Black's opinions and I would like to start using it, preferably without making any changes to its defaults. Why use an opinionated formatter if we want to quibble with its opinions? I set up Black and flake8 yesterday with pre-commit for suitcase-xdi and it works well. I would be very happy to add isort to the mix. In fact I think it would make sense to use isort on all bluesky projects right away.

teddyrendahl commented 5 years ago

I just tried black on a new project, have to say I don't think it really did enough to warrant the time it would take to invest with pre-commit hooks e.t.c. I had already linted with flake8 and all it really seemed to do was stretch out my imports and functions definitions. That being said I'm not against it, but wasn't blown away by how my code was organized afterwards.

isort was useful and very non-invasive.

I certainly fall into the majority mentioned by @danielballan that flake8 should be part of the CI of all these projects. Relieving code reviewers from manually inspecting and commenting on files allows discussion to focus on the implications of code additions instead of being drowned out by whitespace complaints. I also believe the automated checking relieves anyone from having to be the "bad cop" which helps group morale in general.

danielballan commented 5 years ago

I think I am exactly where you are @teddyrendahl: I am at best ambivalent about what it does to the code compared with the results manual flake8 compliance, but becoming open to how it might improve the process.

jklynch commented 5 years ago

I was introduced to the idea of a "diff-minimizing format" by the Elm language style and it seemed pretty strange to me. But I was persuaded that this is actually a very good criterion for programming style because of (what to me anyhow were) the unexpected benefits. First, code gets spread a little bit more vertically than it might otherwise and I as a result I feel like I can read it faster. I also feel like I can give variables and functions slightly longer names because some extra horizontal space is available. Language syntax is emphasized by this style because it tries to anticipate common code modifications, for example adding and removing imports, function arguments, and variables.

As far as improving process goes the main benefit I get is less time thinking about purely stylistic aspects of code, since with an automatic formatter I spend no time on that. Maybe using an automatic formatter removes a small obstacle to new contributors since they don't have to analyze the project just to feel like they can write in the project's style, and they are less likely to be put off by style choices if they don't have to train their fingers and brains to use them. Of course for python code PEP8 is understood to be a baseline style so there is arguably little to re-learn or un-learn from project to project, but handing off the chore of enforcing style from developers to software, as @teddyrendahl as already pointed out, removes "style policing" from code reviews. So using any automatic formatter seems like a good choice for engineering a collaborative and inviting project.

https://www.youtube.com/watch?v=vo9AH4vG2wA