In theory, I would definitely prefer if all the code were auto-formatted
during regular development(*), but in practice the performance means
it's not as transparent as it needs to be -- I have noticed that since
enabling the format-source extension, rebasing my patch stacks is
significantly slower. That's turning out to be a problem because it
tends to take long enough to trigger a context switch, which is
especially problematic when attempting to land something, wandering off
during the rebase, and then realizing that too much time has elapsed and
I'm going to need to start over and pull again because new changes have
come in. (Maybe this just means I should get friendly with Lando, but
even then I tend to have quite a few rebases that I want to tend to
manually while in the testing/debugging phase.)
I know we collect build time statistics when running `mach build` that
we can opt into uploading; would it be possible to collect timings for
eg `hg rebase` operations? Some fancy new gps extension? :-) You
probably shouldn't trust my gut "it feels slower" impressions too far.
- sfink
(*) Now that we've taken the hit of uglifying the code, it seems like we
should get as much mileage as we can out of it.
(I still think it was the right decision to go to a single style, and it
was better to just pick one rather than blocking on arguing it down to a
some "globally optimal" style first.)
On 12/14/2018 10:57 AM, Sylvestre Ledru wrote:
I think we should aim at option b) (updated automatically by bots
after submission to Phabricator)
We have more and more tools at review phase (clang-format, flake8,
eslint, clang-tidy, codespell, etc) which propose some auto-fixes.
Currently, the turn around time of the tools is 14m on average which
is usually faster than the reviewer looking at the patch.
If Phabricator provides the capability, we could have the bot
automatically proposing a new patchset on top on the normal one.
The reviewer would look at the updated version.
By doing that, we would save time for everyone. The main drawback
would be that developer would have to retrieve the updated patch
before updating it.
Sylvestre
Le 13/12/2018 à 23:53, Ehsan Akhgari a écrit :
Previously I had shied away from ideas similar to (a) or (b) since I
wasn't sure if that would be what developers would expect and accept,
given that it would cause the change the reviewer sees to no longer
match their local change, which could cause hicups e.g. when trying
to find the code that a reviewer has commented on locally, or when
rebasing on top of a newer version of mozilla-central after Lando has
landed your patch (which may cause conflicts with your local patch
due to formatting differences).
Do we think these types of issues aren't something we should be
concerned about?
Thanks,
Ehsan
On Fri, Dec 7, 2018 at 3:09 PM Andrew Halberstadt <a...@mozilla.com
<mailto:a...@mozilla.com>> wrote:
I think we should implement a) and do the formatting prior to
submission. This prevents us from wasting reviewer time on format
issues, and also makes sure that "what you see in phab, is what
gets landed".
On Fri, Dec 7, 2018, 2:04 PM Gregory Szorc, <g...@mozilla.com
<mailto:g...@mozilla.com>> wrote:
On Fri, Dec 7, 2018 at 1:57 PM Botond Ballo
<bba...@mozilla.com <mailto:bba...@mozilla.com>> wrote:
> On Fri, Dec 7, 2018 at 11:36 AM Sylvestre Ledru
<sle...@mozilla.com <mailto:sle...@mozilla.com>>
> wrote:
> > In the meantime, we will be running a bot weekly to
reformat the
> > mistakes and add the changeset into the ignore lists.
> > But in the long run this won’t be sustainable, so once we
gain
> > confidence that a good number of developers have
successfully integrated
> > clang-format into their local workflow, we will look into
enabling a
> > Mercurial hook on hg.mozilla.org <http://hg.mozilla.org>
to reject misformatted code upon push
> > time. That will be the ultimate solution to help ensure
that our code
> > will be properly formatted at all times.
>
> Have you considered something like running clang-format
automatically
> during landing (i.e. as part of what Lando does to the
patch)? That
> seems like it would achieve the best of both worlds, by not
placing
> any requirements on what developers do locally, while also
ensuring
> the tree is always properly formatted without cleanup commits.
>
I chatted with Sylvestre earlier today. While I don't want to
speak for
him, I believe we both generally agree that the formatting
should happen
"automagically" as part of the patch review and landing
lifecycle, even if
the client doesn't have their machine configured for
formatting on save.
This would mean that patches are either:
a) auto-formatted on clients as part of being submitted to
Phabricator
b) updated automatically by bots after submission to Phabricator
c) auto-formatted by Lando as part of landing
Lando rewrites/rebases commits as part of landing, so commit
hashes already
change. So if auto-formatting magically occurs and "just
works" as part of
the review/landing process, there should be little to no
developer
inconvenience compared to what happens today. i.e. developers
don't need to
do anything special to have their patches land with proper
formatting.
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
<mailto:dev-platform@lists.mozilla.org>
https://lists.mozilla.org/listinfo/dev-platform
--
Ehsan
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
