Renato Golin via llvm-dev
2021-Apr-23 20:35 UTC
[llvm-dev] Anyone doing code reviews via mailing lists?
Yeah, I can't remember the last time I did a review by email. But I'm sure that they were replies to a commit that broke our bots, and not an actual review of a pull request. Worse, I think if someone does a review by email and doesn't copy me directly, I won't even notice. On Fri, 23 Apr 2021, 19:08 Chris Lattner via llvm-dev, < llvm-dev at lists.llvm.org> wrote:> I think it makes sense to phase out the email review path as well - it is > better to have a single way to do things, and Phabricator is where our > center of gravity is. > > -Chris > > On Apr 23, 2021, at 10:57 AM, Krzysztof Parzyszek via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > > Thanks for the feedback. I didn’t take accessibility concerns into > account. I’ll make sure to mention it if I decide to create a proposal > (which seems likely). > > -- > Krzysztof Parzyszek kparzysz at quicinc.com AI tools development > > *From:* Philip Reames <listmail at philipreames.com> > *Sent:* Friday, April 23, 2021 12:26 PM > *To:* Krzysztof Parzyszek <kparzysz at quicinc.com>; llvm-dev < > llvm-dev at lists.llvm.org> > *Subject:* [EXT] Re: [llvm-dev] Anyone doing code reviews via mailing > lists? > > > If you want to propose this, please move it to a top level thread for > visibility. > > (My thoughts inline to help you refine your proposal.) > On 4/23/21 9:55 AM, Krzysztof Parzyszek via llvm-dev wrote: > > I think we should phase-out email reviews in favor of doing it directly > via Phabricator. > > My reasons: > > 1. Phabricator allows for both pre- and post-commit reviews. You can > “raise concern” with any commit, including those that did not have a > pre-commit review on phab. > 2. The email-phabricator integration is still deficient despite a lot > of effort having been put into it. Moreover, it’s not likely that it will > ever be fully functional. > 3. Email communication is “fragile”. This one is based more on my > personal experience, but even simply following a discussion on llvm-dev has > been difficult. I now have to use Outlook (corporate reasons…) and Outlook > fails at keeping email threads together. Replies to the same tread are > scattered into a mini-forest instead staying as a single tree. There are > issues with every email client formatting the replies differently: top-post > mixed with bottom-post mixed with inline text, with people quoting 500 > lines of text only to insert a single-line response, and so on, and so > forth. Some of it is due to my use of Outlook, some of it is independent > from it. This “infinite flexibility” of email structure is the reason why > I doubt that the Phabricator integration will ever work. > > Phabricator is also fragile unfortunately. I find phabricators inline > discussions to be very hard to follow for anything which becomes involved. > As such, I tend to default to phab, but quickly move to email if discussion > gets complicated. > > > > 1. Phabricator’s interface makes every review look the same, is > readable, and doesn’t make it easy to unintentionally clutter it with junk. > > As someone with vision restrictions, please be careful about this line of > argument. One of the major advantages of email is that I can use my own > client at whatever zoom/scale I want. Phab "sorta works" from an > accessibility perspective, but frankly is inferior to plain old email. I > frequently end up reading phabricator emails, and then replying through the > web interface. > > I'm just point this out because I find that visual appear is often rated > much more highly by some folks than others. Personally, functionality is > pretty much the only thing I care about. > > > 1. Finally, it seems like nowadays it’s easier to create a Phabricator > account than to sign up to the mailing lists… > > > > > -- > Krzysztof Parzyszek kparzysz at quicinc.com AI tools development > > *From:* llvm-dev <llvm-dev-bounces at lists.llvm.org> > <llvm-dev-bounces at lists.llvm.org> *On Behalf Of *Mehdi AMINI via llvm-dev > *Sent:* Friday, April 23, 2021 11:01 AM > *To:* Hubert Tong <hubert.reinterpretcast at gmail.com> > <hubert.reinterpretcast at gmail.com> > *Cc:* llvm-dev <llvm-dev at lists.llvm.org> <llvm-dev at lists.llvm.org> > *Subject:* [EXT] Re: [llvm-dev] Anyone doing code reviews via mailing > lists? > > > > On Fri, Apr 23, 2021 at 8:57 AM Hubert Tong via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > > On Fri, Apr 23, 2021 at 11:25 AM Christian Kühnel via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > > So it looks like the information we have on the mailing list and in > Phabricator is diverging, as those emails do not get parsed back into > Phabricator. > > > I recall noticing that Phabricator also doesn't emit inline code change > suggestions into the e-mail record. > > > Yes: which is an indication that not all the content is on the > mailing-list either :( > > -- > Mehdi > > > > > > _______________________________________________ > > LLVM Developers mailing list > > llvm-dev at lists.llvm.org > > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev > > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210423/1ebf3f4a/attachment.html>
Martin Storsjö via llvm-dev
2021-Apr-23 20:43 UTC
[llvm-dev] Anyone doing code reviews via mailing lists?
On Fri, 23 Apr 2021, Renato Golin via llvm-dev wrote:> Yeah, I can't remember the last time I did a review by email. But I'm sure > that they were replies to a commit that broke our bots, and not an actual > review of a pull request. > Worse, I think if someone does a review by email and doesn't copy me > directly, I won't even notice.Especially for cases where a commit didn't go through review on Phabricator, it's probably common to react to it via e.g. a reply to the commit mail, as it both goes to the list and hopefully reaches the author that way. // Martin