WARNING: bugzilla "review granted" emails no longer contain review comments

classic Classic list List threaded Threaded
34 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Re: WARNING: bugzilla "review granted" emails no longer contain review comments

Ralph Giles-7
On 2014-10-17 10:16 AM, Jim Porter wrote:

> Seeing alta88 complain about someone being negative/dismissive is the
> height of comedy.

The behaviour of a contributor has no bearing on whether they, and those
reading the bug later, deserve respectful and professional replies. The
one does not excuse the other, and neither supports our goal of making
Mozilla a more welcoming place.

 -r
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: WARNING: bugzilla "review granted" emails no longer contain review comments

Jim Porter
In reply to this post by Philip Chee
I'd like to apologize for my previous message to the list, which was
rather, shall we say, blunt. (If you can't tell, and I won't quote it,
it was intended to be a private message.)

Since I'm a submodule owner for Thunderbird, I try to remain neutral in
these things for the most part. In my attempts to defend jcranmer, I
failed pretty thoroughly in that goal. Nevertheless, I try always to be
someone that *any* contributor can rely on for guidance, even if I don't
get along with them personally.

However, I would ask that contributors (not just alta88 or jcranmer, but
several others who will remain nameless) try to remember that the other
commenters are people too, and that things work a lot better if you just
try to get along. I've seen far too many campaigns against other
members, and I frankly don't want to get involved in them. I only got
involved in this case because I felt jcranmer was being unfairly targeted.

I'm not sure what the best way to make people on Bugzilla get along
(especially in Thunderbird, where we're a bit ramshackle at times), and
even if I knew, I don't think I have the time to do much about it.
Still, if you're reading this, please do remember that even if you just
have a bone to pick with one other member, it tends to drag other people
into it (like me).

Sorry again.

- Jim
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: WARNING: bugzilla "review granted" emails no longer contain review comments

Steve Fink-4
In reply to this post by Gavin Sharp-3
On 10/17/2014 09:46 AM, Gavin Sharp wrote:
> mhoye already said it, but I'll say it too: Bugzilla interactions that
> denigrate well-meaning contributors, like the one in that example, are
> not acceptable and certainly shouldn't be celebrated. I think an
> apology is in order.

I skimmed the thread in question. I would suggest that nobody jump to
conclusions about this particular interaction without doing so. I saw
some misunderstandings, and not a lot of good faith effort to resolve
the miscommunication, but the whole thing felt more like a tempest in a
teapot.

As for the specific quoted passage, it was obvious to me that jcranmer
was referring to the current state of the web and our code base, and was
not mocking any person. At the same time, it was easy to see how someone
might interpret it that way.

There's some amount of blame to spread around, but from my reading of
that thread, nobody is clearly in the wrong. Being pleasant and
respectful of contributors *is* extremely important, and we should be
going out of our way to do that, but this thread is not one to hang your
hat on.
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: WARNING: bugzilla "review granted" emails no longer contain review comments

Gavin Sharp-3
I should clarify, because I think my asking for an apology is likely
to be misinterpreted.

- Clearly, there's a lot of context behind this specific example that
needs to be considered. I was aware of some of it (I'm familiar with
alta88 and jcranmer, and I did read the bug thread), but not all of
it.
- We are substantially off-topic given this thread's subject - this
example is not related to a review comment. This was perhaps not the
best place for alta88 to raise this concern.
- Appropriateness of venue aside, after it was raised here, Philip's
reply condoning the example out of context had a high likelihood of
being interpreted as a more general condoning of this type of behavior
in Bugzilla in general. I thought it important to make it clear that
it is not acceptable behavior in general.
- Regardless of the mitigating factors (which certainly exist) and
broader context, I think Joshua recognizes that that particular bug
comment was not the best way to communicate what he wanted to
communicate - that's what I was suggesting he apologize for, nothing
more.

Gavin

On Fri, Oct 17, 2014 at 11:28 AM, Steve Fink <[hidden email]> wrote:

> On 10/17/2014 09:46 AM, Gavin Sharp wrote:
>> mhoye already said it, but I'll say it too: Bugzilla interactions that
>> denigrate well-meaning contributors, like the one in that example, are
>> not acceptable and certainly shouldn't be celebrated. I think an
>> apology is in order.
>
> I skimmed the thread in question. I would suggest that nobody jump to
> conclusions about this particular interaction without doing so. I saw
> some misunderstandings, and not a lot of good faith effort to resolve
> the miscommunication, but the whole thing felt more like a tempest in a
> teapot.
>
> As for the specific quoted passage, it was obvious to me that jcranmer
> was referring to the current state of the web and our code base, and was
> not mocking any person. At the same time, it was easy to see how someone
> might interpret it that way.
>
> There's some amount of blame to spread around, but from my reading of
> that thread, nobody is clearly in the wrong. Being pleasant and
> respectful of contributors *is* extremely important, and we should be
> going out of our way to do that, but this thread is not one to hang your
> hat on.
> _______________________________________________
> dev-planning mailing list
> [hidden email]
> https://lists.mozilla.org/listinfo/dev-planning
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: WARNING: bugzilla "review granted" emails no longer contain review comments

Edmund Wong-2
In reply to this post by Steve Fink-4
Gavin Sharp wrote:
> I should clarify, because I think my asking for an apology is likely
> to be misinterpreted.
>
I'm not good with social interactions so please pardon if I've
stepped on anyone's toes, fingers, extremeties...

I am a contributor. Not a new one; but a relative newcomer.
I've contributed to a few areas, but I'm mainly a SeaMonkey dev.
(I've submitted patches to Firefox, Chatzilla, Pulse, Bugzilla,
etc..)

That said, I feel that the r- state is disconcerting; but not
necessarily offensive.  When I was a relatively 'new' contributer,
I had issues with that flag.  To me, that 'r-' said, "Dude,
your coding sucks.  You suck. Give up already!"
No, that's not what the reviewers said; that's what I got from
that flag.  Maybe it's just me.

Even after these few years of having my patches reviewed,
I still can't get the hang of that 'r-' state.  It's supposed
to mean "good try.. fix the code and try again' and not to mean
'what the smeg are you thinking, you dork.' It's the connotation
of the '-' sign.  +ve = good. -ve = bad.

Is there a better way of doing this? Probably. The stigma of
-ve will still be there.  Change the symbols?  Change how
'r-' is given?  I mean, 'r-' should be given to those patches
that really is wrong and no amount of 'touch up' will
fix it. (i.e. fixing a bug that needs a change in foo.cpp
but the contributor made irrelevant changes in bar.js)  r^ could
replace the current 'r-' such that patches that can be improved would
be given that.  Sure, it doesn't avoid the stigma of 'r-', it just
limits the actual # of 'r-' contributors get. (Mainly a psychological
thing.)

Just my unsolicited $0.02.

Edmund
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: WARNING: bugzilla "review granted" emails no longer contain review comments

Henri Sivonen-2
In reply to this post by Gavin Sharp-3
On Wed, Oct 15, 2014 at 10:05 PM, Gavin Sharp <[hidden email]> wrote:
> "you are not your r-'d patch" is a fine stance to use with review
> requesters that you are familiar with (and who are very familiar with
> our processes), but you can't rely on it being adopted by e.g. new
> contributors, or people used to slightly different processes.

In that case, we really need to fix Bugzilla.

r- is a state you need especially with new contributors, because you
probably can't trust them to correctly execute "r+ if you fix X, Y and
Z" while you probably can trust someone you've worked with for years
not to need further review when told "r+ if you fix X, Y and Z".

(When I use r- on a patch from someone new, I try to explain that it's
generally OK but it's r- just because it's not 100% right yet.)

--
Henri Sivonen
[hidden email]
https://hsivonen.fi/
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: WARNING: bugzilla "review granted" emails no longer contain review comments

Gijs Kruitbosch ("Hannibal")
In reply to this post by Edmund Wong-2
On 20/10/2014 05:01, Edmund Wong wrote:

> Gavin Sharp wrote:
>> I should clarify, because I think my asking for an apology is likely
>> to be misinterpreted.
>>
> I'm not good with social interactions so please pardon if I've
> stepped on anyone's toes, fingers, extremeties...
>
> I am a contributor. Not a new one; but a relative newcomer.
> I've contributed to a few areas, but I'm mainly a SeaMonkey dev.
> (I've submitted patches to Firefox, Chatzilla, Pulse, Bugzilla,
> etc..)
>
> That said, I feel that the r- state is disconcerting; but not
> necessarily offensive.  When I was a relatively 'new' contributer,
> I had issues with that flag.  To me, that 'r-' said, "Dude,
> your coding sucks.  You suck. Give up already!"
> No, that's not what the reviewers said; that's what I got from
> that flag.  Maybe it's just me.
>
> Even after these few years of having my patches reviewed,
> I still can't get the hang of that 'r-' state.  It's supposed
> to mean "good try.. fix the code and try again' and not to mean
> 'what the smeg are you thinking, you dork.' It's the connotation
> of the '-' sign.  +ve = good. -ve = bad.
>
> Is there a better way of doing this? Probably. The stigma of
> -ve will still be there.  Change the symbols?  Change how
> 'r-' is given?  I mean, 'r-' should be given to those patches
> that really is wrong and no amount of 'touch up' will
> fix it. (i.e. fixing a bug that needs a change in foo.cpp
> but the contributor made irrelevant changes in bar.js)

I see people use feedback- for this meaning (ie the "approach of the
patch is wrong, start over please" rather than "this is generally good
but you also need to do X/Y/Z, come back when you've also done that". I
tend to use feedback+ for the things you would like to get "r^". YMMV
with different reviewers.


   r^ could
> replace the current 'r-' such that patches that can be improved would
> be given that.  Sure, it doesn't avoid the stigma of 'r-', it just
> limits the actual # of 'r-' contributors get. (Mainly a psychological
> thing.)

While this fixes the psychological thing, it introduces a psychological
issue in that "r^" has no intuitive meaning.

Or maybe that's my having been here for way too long, and therefore
thinking of "review+" as intuitive when it really isn't so to new
contributors? I dunno.

~ Gijs
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: WARNING: bugzilla "review granted" emails no longer contain review comments

Gavin Sharp-3
In reply to this post by Henri Sivonen-2
My post was not intended to have the takeaway "never use r- for new
contributors". I was pushing back on "everyone should be able to deal
with a brusque r-". Of course it's reasonable to use r- on
new-contributor patches, as long as the overall message communicated
is helpful rather than dismissive. That currently usually requires
more than just setting a flag, but maybe it could be simplified to
that with some Bugzilla tweaks (additional review flag states, or
better explanations for what the flags imply).

Gavin

On Mon, Oct 20, 2014 at 12:43 AM, Henri Sivonen <[hidden email]> wrote:

> On Wed, Oct 15, 2014 at 10:05 PM, Gavin Sharp <[hidden email]> wrote:
>> "you are not your r-'d patch" is a fine stance to use with review
>> requesters that you are familiar with (and who are very familiar with
>> our processes), but you can't rely on it being adopted by e.g. new
>> contributors, or people used to slightly different processes.
>
> In that case, we really need to fix Bugzilla.
>
> r- is a state you need especially with new contributors, because you
> probably can't trust them to correctly execute "r+ if you fix X, Y and
> Z" while you probably can trust someone you've worked with for years
> not to need further review when told "r+ if you fix X, Y and Z".
>
> (When I use r- on a patch from someone new, I try to explain that it's
> generally OK but it's r- just because it's not 100% right yet.)
>
> --
> Henri Sivonen
> [hidden email]
> https://hsivonen.fi/
> _______________________________________________
> dev-planning mailing list
> [hidden email]
> https://lists.mozilla.org/listinfo/dev-planning
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: WARNING: bugzilla "review granted" emails no longer contain review comments

mhoye

On 2014-10-20 11:31 AM, Gavin Sharp wrote:
> My post was not intended to have the takeaway "never use r- for new
> contributors". I was pushing back on "everyone should be able to deal
> with a brusque r-". Of course it's reasonable to use r- on
> new-contributor patches, as long as the overall message communicated
> is helpful rather than dismissive.
I suspet  can get out in front of this with some better documentation of
what the code-review process looks like in our introduction page.

https://developer.mozilla.org/en-US/docs/Introduction

...and by making it clear that an r-  is a technical note about patch
readiness, and not an evaluation of your effort or character. So, I'll
do that.

The other thing I can do, since I'm already surveying new contributors
for other reasons, is some informal sentiment analysis and ask them what
their real impressions of our processes are. It may be that we're more
concerned about this than the data warrants! And it may not, obvs.

Anyway, I'll do both of those things and report back. While I'm at it,
let me throw this out there: If you had one question you'd like to ask
our new contributors, what would it be?

- mhoye



_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: WARNING: bugzilla "review granted" emails no longer contain review comments

julian.viereck
In reply to this post by Gavin Sharp-3
On Monday, October 20, 2014 5:46:08 PM UTC+2, Mike Hoye wrote:
> I suspet  can get out in front of this with some better documentation of
>
> what the code-review process looks like in our introduction page.

Could bugzilla show an inline notification like "What does r- mean?" next to the comments to an r- in case it detects it's the first patch from a new contributor?

On 20/10/2014 05:01, Edmund Wong wrote:
> replace the current 'r-' such that patches that can be improved would
> be given that.  Sure, it doesn't avoid the stigma of 'r-', it just
> limits the actual # of 'r-' contributors get. (Mainly a psychological
> thing.)

Personally, I like the thinking about making "r-" more psychological friendly. Maybe use `r0` to indicate it's "neutral" or "r+-" or "r+/-"?
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: WARNING: bugzilla "review granted" emails no longer contain review comments

Steve Fink-4
On 10/20/2014 05:15 PM, Julian Viereck wrote:
> On 20/10/2014 05:01, Edmund Wong wrote:
>> replace the current 'r-' such that patches that can be improved would
>> be given that.  Sure, it doesn't avoid the stigma of 'r-', it just
>> limits the actual # of 'r-' contributors get. (Mainly a psychological
>> thing.)
> Personally, I like the thinking about making "r-" more psychological friendly. Maybe use `r0` to indicate it's "neutral" or "r+-" or "r+/-"?

"r = not yet"

_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: WARNING: bugzilla "review granted" emails no longer contain review comments

mhoye
In reply to this post by mhoye
On 2014-10-20 11:46 AM, Mike Hoye wrote:
>
>
>
> ...and by making it clear that an r-  is a technical note about patch
> readiness, and not an evaluation of your effort or character. So, I'll
> do that.
And there we go.

https://developer.mozilla.org/en-US/docs/Introduction



- mhoye
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: WARNING: bugzilla "review granted" emails no longer contain review comments

Ralph Giles-7
On 2014-10-30 9:22 AM, Mike Hoye wrote:

> https://developer.mozilla.org/en-US/docs/Introduction

This is really great, thanks!

One question about 'Step 6'. The page says,

  Mark that your patch is ready to commit by adding the
  checkin-needed keyword to the "whiteboard" field at
  the top of the bug. A friendly Mozillian with commit
  access will be along shortly to push your patch to
  the repository, ...

The process I'm familiar with is _if_ there is a green try push and one
puts "checkin-needed" in the _keyword_ (not whiteboard) field, a sheriff
will push the patch to inbound. Which is wonderful, but leaves
contributors without try access or a mentor few options if e.g. the
reviewer doesn't take care of landing for them.

Has our process here changed? (If so, I'd like to use it myself whenever
a patch isn't urgent or inbound is closed!) If not, how can we make sure
no one's patches hang around in checkin-needed too long?

 -r
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: WARNING: bugzilla "review granted" emails no longer contain review comments

mhoye
On 2014-10-30 1:57 PM, Ralph Giles wrote:

>   at
>    the top of the bug. A friendly Mozillian with commit
>    access will be along shortly to push your patch to
>    the repository, ...
>
> The process I'm familiar with is_if_  there is a green try push and one
> puts "checkin-needed" in the_keyword_  (not whiteboard) field, a sheriff
> will push the patch to inbound. Which is wonderful, but leaves
> contributors without try access or a mentor few options if e.g. the
> reviewer doesn't take care of landing for them.
>
> Has our process here changed?
No, that's an error on my part. I'll correct it directly.

- mhoye
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
12