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 concerned about?


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
        > 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
        > 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
        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
        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
        dev-platform mailing list

dev-platform mailing list

Reply via email to