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.
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
On Fri, Dec 7, 2018 at 3:09 PM Andrew Halberstadt <a...@mozilla.com
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
On Fri, Dec 7, 2018, 2:04 PM Gregory Szorc, <g...@mozilla.com
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
> > In the meantime, we will be running a bot weekly to
> > 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
> > clang-format into their local workflow, we will look into
> > 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
> during landing (i.e. as part of what Lando does to the
> seems like it would achieve the best of both worlds, by not
> any requirements on what developers do locally, while also
> the tree is always properly formatted without cleanup commits.
I chatted with Sylvestre earlier today. While I don't want to
him, I believe we both generally agree that the formatting
"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
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
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
dev-platform mailing list
dev-platform mailing list