This is an archive of the discontinued Mercurial Phabricator instance.

style: run black on a subset of mercurial
Needs RevisionPublic

Authored by mjpieters on Oct 13 2018, 6:07 AM.

Details

Reviewers
indygreg
durin42
baymax
Group Reviewers
hg-reviewers
Summary

This applied black to the 20 smallest files in mercurial/:

ls -S1 mercurial/*.py | tail -n20 | xargs black

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

mjpieters created this revision.Oct 13 2018, 6:07 AM

The import parentheses collapsing is described here: https://github.com/ambv/black#parentheses. The behavior is not configurable.

mjpieters updated this revision to Diff 12047.Oct 13 2018, 7:56 AM
mjpieters updated this revision to Diff 12059.Oct 13 2018, 8:39 AM
indygreg added inline comments.Oct 13 2018, 9:23 AM
contrib/check-commit
43

I think we'll want to send this chunk as a separate commit, as it is a policy change. I'd be happy to accept that patch today.

mjpieters updated this revision to Diff 12135.Oct 14 2018, 9:41 AM

Where do we stand on the intent to mass reformat the code base?

I'm not super thrilled at some of black's decisions (like using double quotes for all strings and merging imports into the same line which leads to excessive code churn later). But the exact style doesn't matter as much as having it be consistent. So I'm willing to go with black if we feel it's the best tool for the job. I'm not sure what the alternatives are these days.

mercurial/mergeutil.py
18

I'm surprised by this result. I'd like to think the reformatting tool would be smarter than this.

av6 added a subscriber: av6.Nov 14 2018, 1:41 AM

I look at the changes and see nitpicks at best. On the one hand, black proved better than any linter that we can already write consistent code. On the other, if black were a linter... I'd switch to flake8, which at least is configurable.

In D5064#78545, @av6 wrote:

I look at the changes and see nitpicks at best. On the one hand, black proved better than any linter that we can already write consistent code. On the other, if black were a linter... I'd switch to flake8, which at least is configurable.

The whole point of black is that it is not configurable. Configurable means you still have to argue about style and decide on a configuration.

In D5064#78545, @av6 wrote:

I look at the changes and see nitpicks at best. On the one hand, black proved better than any linter that we can already write consistent code. On the other, if black were a linter... I'd switch to flake8, which at least is configurable.

The whole point of black is that it is not configurable. Configurable means you still have to argue about style and decide on a configuration.

Yep. I find I care less about style decisions when the computer is able to make them for me: eg I prefer ' to ", but since black can fix that and I don't have to type the " myself, I don't really care.

Does anyone strongly object, or do we want to embrace black and the formatting changes it implies?

Does anyone strongly object, or do we want to embrace black and the formatting changes it implies?

Is there a fix extension configuration (and I guess precommit hook- I've never used it) for this? If so, maybe it should go on the ContributingChanges page.

Does anyone strongly object, or do we want to embrace black and the formatting changes it implies?

Is there a fix extension configuration (and I guess precommit hook- I've never used it) for this? If so, maybe it should go on the ContributingChanges page.

My plan was to put an hg fix configuration in contrib and mention it in ContributingChanges. I'd advise against a hook for this, but if someone else wanted to write one I'd at least drop it in contrib as an example...

Per mailing list thread, I've sent out https://phab.mercurial-scm.org/D5539 to show what yapf would want to do.

baymax requested changes to this revision.Jan 24 2020, 12:31 PM

There seems to have been no activities on this Diff for the past 3 Months.

By policy, we are automatically moving it out of the need-review state.

Please, move it back to need-review without hesitation if this diff should still be discussed.

:baymax:need-review-idle:

This revision now requires changes to proceed.Jan 24 2020, 12:31 PM