Re: Phabricator and Bugzilla
On Monday, 9 April 2018 15:29:35 UTC-4, Randell Jesup wrote: > >As I indicated, those posts go into detail on why we are avoiding both > >comment and more complicated flag mirroring. > > > >Mark > > There's no obvious discussion of "flags" in the linked discussions you > gave; I find only a couple of references to "flag" - in a question from > gps. Given how long the thread is (52 posts), perhaps you can point to > something more specific? Thanks gps's post is what I was talking about. It doesn't go into many details, but, to reiterate his main point, the mapping of disparate review systems is difficult and has lots of edge cases. To add a bit of specifics, the models behind review flags in BMO, which are independent objects associated with specific attachments, is very different from the way Phabricator treats reviews, which are particular states of a revision. It's doubtful we could ever have a perfect translation, so it would always be some sort of a hack. We've determined that a better solution is to expose Phabricator requests in Bugzilla as notifications and dashboards, as I linked in one of my replies above. Mark > > >On Sat, Mar 31, 2018 at 10:14 AM, Ben Kelly wrote: > > > >> On Sat, Mar 31, 2018, 10:09 AM Mark Côté 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. > > -- > Randell Jesup, Mozilla Corp > remove "news" for personal email ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Phabricator and Bugzilla
>As I indicated, those posts go into detail on why we are avoiding both >comment and more complicated flag mirroring. > >Mark There's no obvious discussion of "flags" in the linked discussions you gave; I find only a couple of references to "flag" - in a question from gps. Given how long the thread is (52 posts), perhaps you can point to something more specific? Thanks >On Sat, Mar 31, 2018 at 10:14 AM, Ben Kelly wrote: > >> On Sat, Mar 31, 2018, 10:09 AM Mark Côté 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. -- Randell Jesup, Mozilla Corp remove "news" for personal email ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Phabricator and Bugzilla
As I indicated, those posts go into detail on why we are avoiding both comment and more complicated flag mirroring. Mark On Sat, Mar 31, 2018 at 10:14 AM, Ben Kelly wrote: > On Sat, Mar 31, 2018, 10:09 AM Mark Côté 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
Re: Phabricator and Bugzilla
On Sat, Mar 31, 2018, 10:09 AM Mark Côté 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
Re: Phabricator and Bugzilla
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). 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