On Sat, Mar 31, 2018, 10:09 AM Mark Côté <mc...@mozilla.com> wrote:
> Regarding comment and flag mirroring, we've discussed this before: > > > https://groups.google.com/d/msg/mozilla.dev.platform/Y8kInYxo8UU/e3Pi-_FpBgAJ > > https://groups.google.com/d/msg/mozilla.dev.platform/Y8kInYxo8UU/tsF7UfxvBgAJ > > Given that Phabricator is still new, I don't see any reason to reopen that > discussion at this point, aside from noting that we have work in progress > to include Phabricator requests in BMO's My Dashboard and notifications > indicator (https://bugzilla.mozilla.org/show_bug.cgi?id=1440828). > What about comment mirroring? On my mobile so I haven't read all the past threads, but my recollection is that your team did not want to implement that feature. Personally, this is a huge concern for me. Thanks. Ben > As for interdiffs, feel free to file a bug with any problems you see. We > have a good relationship with upstream and can pass those on. Similarly > with method names (which has been mentioned before but I can't find where > at the moment). > > There is official documentation at > https://secure.phabricator.com/book/phabricator/ which is linked from our > Mozilla-specific docs ( > http://moz-conduit.readthedocs.io/en/latest/phabricator-user.html) which > in turn is linked in the left-hand menu in Phabricator. We can expand our > own docs as needed if there are areas that are particularly confusing due > to, say, expectations carried over from our other code-review tools. > > Mark > > On Thursday, 29 March 2018 13:45:28 UTC-4, smaug wrote: > > Hi all, > > > > just some random notes about Phabricator. > > > > I've been reviewing now a bunch of patches in Phabricator and the > initial feeling from > > reviewer's point of view is that it is ok. Not great, but ok. > > MozReview's interdiff, when it works, is easier to use or at least to > discover than Phabricator's. > > MozReview does also show the method name in which the code lives. This > latter thing is actually a big > > + to MozReview. (Same works in Bugzilla too when reviewing raw -P > patches at least. No idea about splinter, since I never use it). > > If Pabricator has the feature, I haven't found it yet - its UI is a tad > confusing occasionally. > > > > What is currently rather broken is the flag synchronization between > bugzilla and Phabricator. One doesn't see in Bugzilla whether review has > been > > asked or whether review has been denied (r-). I believe people asking > reviews from me set the r? flag manually in bugzilla. > > Having an easy way to see that r- has been given to a patch is very > valuable to the reviewer when looking at the bug again. > > Oddly enough, approving the patch in Phabricator does set r+ in Bugzilla. > > > > Not mirroring comments from Phabricator to Bugzilla has already made > following reasoning for certain code changes harder. > > It is so easy to include non-code level information in review comments. > So far I haven't had a case where I would have needed to look at the blame > and > > try to find relevant information both in bugzilla and in phabricator, > but that will obviously happen if we don't do the mirroring and it will slow > > down reviewing in the future (since looking at the history/reasoning for > the code changes is a big part of code reviewing). > > > > I'm sure I haven't found all the tricks Phabricator has to help > reviewing. Is there some reviewing-in-Phabricator-for-dummies doc? > > > > > > I haven't asked reviews in Phabricator (nor in MozReview), so can't > comment on how they compare from patch author's point of view. > > > > > > > > br, > > > > > > > > -Olli > > _______________________________________________ > 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
