Auto-formatting code with black - object now if you have a strong opinion

[Gregory Szorc]

On Thu, Nov 29, 2018 at 1:28 PM Augie Fackler <raf at durin42.com> wrote:

> On Tue, Nov 27, 2018 at 6:42 PM Gregory Szorc <gregory.szorc at gmail.com>
> wrote:
> >
> > On Tue, Nov 27, 2018 at 8:13 AM Augie Fackler <raf at durin42.com> wrote:
> >>
> >>
> >>
> >> > On Nov 27, 2018, at 11:09, Pulkit Goyal <7895pulkit at gmail.com> wrote:
> >> >
> >> > I am +1 on using black.
> >> >
> >> > Adding to what Augie said, we plan to preserve the annotate history
> by commiting the changes with something like "# skip-blame" in commit
> message. We can then skip those revisions while annotating using the
> `--skip` flag of `hg annotate` which accepts a revset, something like `hg
> annotate --skip "desc('# skip-blame')"`.
> >> >
> >> > Maybe we should use "# skip-blame black-changes" to mark those
> changes as black specific and to differentiate between py3 and black
> changes. (We already use "# skip-blame" for py3 related trivial changes.)
>
> >>
> >> Indeed. Note that if we do the black transition, we should probably
> also do the mass-addition of b prefixes on our string literals at the same
> time (in a separate commit, obviously) so that people only have painful
> rebases once and not twice.
> >
> >
> > IMO the annotation should be "# auto-reformat" or something along those
> lines: you should be annotating the action that was taken, not the
> side-effects you purport you want to take from it. When skipping revisions
> for annotate, we can use a revset with all the relevant annotations. e.g.
> `desc("# auto-reformat") | desc("# py3-compat")`. But we've already started
> using "# skip-blame" so perhaps this ship has sailed.
>
> auto-reformat seems fine, I'm happy to have both.
>
> >
> > Speaking of ships that have sailed, I would prefer we use "annotate"
> instead of "blame," as the former doesn't have the negative connotations of
> the latter.
> >
> > I agree with Augie that a mass-addition of b prefixes should be done at
> the same time in a separate commit from reformatting.
> >
> > I have no preferences whether we should have a single massive commit or
> split things up by file.
> >
> > I also have some issues with black's formatting. I dislike black's use
> of double quotes for strings (maybe a holdover from my knowledge of shell,
> Perl, and PHP, where different quotes matter).
>
> > And I wish it didn't collect imports on the same line.
>
> I think we could work around this by having one import per import
> statement, eg
>
> from mercurial import hg
> from mercurial import ui as uimod
>
> I don't remember why we don't do that today. Is it performance or
> aesthetics?
>

Each "import X" or "from X import Y" statement is essentially converted
into a call to __import__(). So having a single statement with multiple
"from" values means fewer calls to __import__(). This in turn means fewer
lookups of the parent package and other actions that are performed in the
bowels of the importer. (Keep in mind we have a custom importer for delay
loading and for source transformation, so there may be a substantial amount
of code that is skipped by coalescing the imports). What that performance
translates to, I'm not sure.

Perhaps someone could run black on the entire source tree and compare the
execution time of a simple command to see if it matters in reality?

We could, of course, rewrite the source code as part of importing (like we
do on Python 3) to the "faster" form if there are performance implications.


>
> > But my objections are insignificant compared to the benefits of having
> consistently formatted source code. The research in this area shows that
> the actual style matters less than consistency. If you really care about a
> style, you can configure your editor to apply your personal source format
> on file load and the official style format on save. You don't get to do
> that on remote code review, hgweb, or when looking at diffs from `hg`
> output. But it's a mitigation if you care that much.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20181129/046b6412/attachment.html>