On Fri, Dec 14, 2018 at 4:55 PM Kartikaya Gupta <kgu...@mozilla.com> wrote:
> On Fri, Dec 14, 2018 at 1:58 PM Sylvestre Ledru <sle...@mozilla.com> > wrote: > > We have more and more tools at review phase (clang-format, flake8, > eslint, clang-tidy, codespell, etc) which propose some auto-fixes. > > Honestly I find it quite annoying when I'm trying to review a patch > and the phabricator diff is filled up with bot comments about > formatting. As a reviewer, I shouldn't have to think about formatting, > and it shouldn't fill up my screen and prevent me from looking at the > actual code. > > > Currently, the turn around time of the tools is 14m on average which is > usually faster than the reviewer looking at the patch. > > Usually, but not always. And 14 minutes is still long enough that the > person writing the patch has context-switched away and is in the > middle of doing something else when the bot comes a-knocking. > What if we *rejected* misformatted patches in Phabricator as opposed to just comment on them? That is, what if the bot would mark the reviews as "Request Changes" when it found any problems, IOW it would review the patch alongside with normal reviewers. As Sylvestre mentioned the diffs on individual lines is about to go away, but this way we'd also make sure that misformatting caught at review time would actually block the landing of 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. > > This would be fine for the reviewer, but... > > > The main drawback would be that developer would have to retrieve the > updated patch > > before updating it. > > That's a chore too. For the developer writing the patch I feel like > there should be a way to just say "please format this patch in-place > for me" (basically what I expected `./mach clang-format` to do, except > it doesn't quite - see bug 1514279) and even that should be opt-in. > IMO the default behaviour should just be to automatically apply > formatting prior to submission, without anybody having to think about > it except in cases where the developer needs to apply "//clang-format > off" blocks. > Right. I think the ideal state would be for everyone to use editor integration and local VCS hooks. The VCS hooks we can enable in ./mach bootstrap but editor integration is something people will need to opt into turning on. Once we get there, then I think formatting will be mostly taken care of transparently for you without needing to think about it consciously. Doing things like rejecting misformatted code at review time may incentivize people updating their local configuration. I'm thinking of reasons why that may break people's workflow but can't think of any right now, curious to know if anyone else can? -- Ehsan _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
