docs: advice to resolve discussion on gitlab MR doc

Submitted by apinheiro on May 16, 2019, 9:35 a.m.

Details

Message ID 20190516093545.29702-1-apinheiro@igalia.com
State New
Headers show
Series "docs: advice to resolve discussion on gitlab MR doc" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

apinheiro May 16, 2019, 9:35 a.m.
For newcomers to gitlab, it is not evident that it is better to press
the "Resolve Discussion" button when you update your branch handling
feedback.
---

As the commit message says, it is not always evident. I was pointed to
do that when I started to use gitlab, and just today I mentioned it to
two different people that didn't know about that.

Having said so, I feel that the specific text needs some poulishing
first, so any suggestion is welcome.

 docs/submittingpatches.html | 4 ++++
 1 file changed, 4 insertions(+)

Patch hide | download patch | download mbox

diff --git a/docs/submittingpatches.html b/docs/submittingpatches.html
index 020e73d09ec..147b97d76e1 100644
--- a/docs/submittingpatches.html
+++ b/docs/submittingpatches.html
@@ -258,6 +258,10 @@  your email administrator for this.)
 </p>
 <ul>
   <li>Make changes and update your branch based on feedback
+  <li>After an update, for the feedback you handled, close the
+  feedback discussion with the button "Resolve Discussion". In this
+  way the reviewer would know which feedback got handled and which
+  not.
   <li>Old, stale MR may be closed, but you can reopen it if you
     still want to pursue the changes
   <li>You should periodically check to see if your MR needs to be

Comments

Some grammar nits:

- "Resolve Discussion" goes before "button" as it modifies it.
- It's either "This way..." or "In this manner...", not "In this
way...", although the latter is a little too stilted/over-formal here.
- This isn't a hypothetical or another situation where "would know..."
is appropriate.
- "...which didn't" (since it's short for "which didn't get handled").

The corrected text is:

After an update, for the feedback you handled, close the feedback
discussion with the "Resolve Discussion" button. This way the reviewer
knows which feedback got handled and which didn't.

I definitely didn't notice this when I started using Gitlab either.
With the fixed text:

Reviewed-by: Connor Abbott <cwabbott0@gmail.com>

On Thu, May 16, 2019 at 11:36 AM Alejandro Piñeiro <apinheiro@igalia.com> wrote:
>
> For newcomers to gitlab, it is not evident that it is better to press
> the "Resolve Discussion" button when you update your branch handling
> feedback.
> ---
>
> As the commit message says, it is not always evident. I was pointed to
> do that when I started to use gitlab, and just today I mentioned it to
> two different people that didn't know about that.
>
> Having said so, I feel that the specific text needs some poulishing
> first, so any suggestion is welcome.
>
>  docs/submittingpatches.html | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/docs/submittingpatches.html b/docs/submittingpatches.html
> index 020e73d09ec..147b97d76e1 100644
> --- a/docs/submittingpatches.html
> +++ b/docs/submittingpatches.html
> @@ -258,6 +258,10 @@ your email administrator for this.)
>  </p>
>  <ul>
>    <li>Make changes and update your branch based on feedback
> +  <li>After an update, for the feedback you handled, close the
> +  feedback discussion with the button "Resolve Discussion". In this
> +  way the reviewer would know which feedback got handled and which
> +  not.
>    <li>Old, stale MR may be closed, but you can reopen it if you
>      still want to pursue the changes
>    <li>You should periodically check to see if your MR needs to be
> --
> 2.19.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Thursday, 2019-05-16 13:14:46 +0200, Connor Abbott wrote:
> Some grammar nits:
> 
> - "Resolve Discussion" goes before "button" as it modifies it.
> - It's either "This way..." or "In this manner...", not "In this
> way...", although the latter is a little too stilted/over-formal here.
> - This isn't a hypothetical or another situation where "would know..."
> is appropriate.
> - "...which didn't" (since it's short for "which didn't get handled").
> 
> The corrected text is:
> 
> After an update, for the feedback you handled, close the feedback
> discussion with the "Resolve Discussion" button. This way the reviewer
> knows which feedback got handled and which didn't.
> 
> I definitely didn't notice this when I started using Gitlab either.
> With the fixed text:
> 
> Reviewed-by: Connor Abbott <cwabbott0@gmail.com>

I agree with the message, and the fixed up text by Connor is:
Reviewed-by: Eric Engestrom <eric.engestrom@intel.com>

If this isn't too bikeshed-y, I would also say "the reviewers know" as
there are potentially (and usually) more than one, but this really
doesn't matter that much.

> 
> On Thu, May 16, 2019 at 11:36 AM Alejandro Piñeiro <apinheiro@igalia.com> wrote:
> >
> > For newcomers to gitlab, it is not evident that it is better to press
> > the "Resolve Discussion" button when you update your branch handling
> > feedback.
> > ---
> >
> > As the commit message says, it is not always evident. I was pointed to
> > do that when I started to use gitlab, and just today I mentioned it to
> > two different people that didn't know about that.
> >
> > Having said so, I feel that the specific text needs some poulishing
> > first, so any suggestion is welcome.
> >
> >  docs/submittingpatches.html | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/docs/submittingpatches.html b/docs/submittingpatches.html
> > index 020e73d09ec..147b97d76e1 100644
> > --- a/docs/submittingpatches.html
> > +++ b/docs/submittingpatches.html
> > @@ -258,6 +258,10 @@ your email administrator for this.)
> >  </p>
> >  <ul>
> >    <li>Make changes and update your branch based on feedback
> > +  <li>After an update, for the feedback you handled, close the
> > +  feedback discussion with the button "Resolve Discussion". In this
> > +  way the reviewer would know which feedback got handled and which
> > +  not.
> >    <li>Old, stale MR may be closed, but you can reopen it if you
> >      still want to pursue the changes
> >    <li>You should periodically check to see if your MR needs to be
> > --
> > 2.19.1
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On 16/5/19 13:35, Eric Engestrom wrote:
> On Thursday, 2019-05-16 13:14:46 +0200, Connor Abbott wrote:
>> Some grammar nits:
>>
>> - "Resolve Discussion" goes before "button" as it modifies it.
>> - It's either "This way..." or "In this manner...", not "In this
>> way...", although the latter is a little too stilted/over-formal here.
>> - This isn't a hypothetical or another situation where "would know..."
>> is appropriate.
>> - "...which didn't" (since it's short for "which didn't get handled").
>>
>> The corrected text is:
>>
>> After an update, for the feedback you handled, close the feedback
>> discussion with the "Resolve Discussion" button. This way the reviewer
>> knows which feedback got handled and which didn't.
>>
>> I definitely didn't notice this when I started using Gitlab either.
>> With the fixed text:
>>
>> Reviewed-by: Connor Abbott <cwabbott0@gmail.com>
> I agree with the message, and the fixed up text by Connor is:
> Reviewed-by: Eric Engestrom <eric.engestrom@intel.com>
>
> If this isn't too bikeshed-y, I would also say "the reviewers know" as
> there are potentially (and usually) more than one, but this really
> doesn't matter that much.


I pushed the patch with Connor text plus this suggestion. Thanks for the 
quick review! (and for basically rewrite my broken grammar english text)

>
>> On Thu, May 16, 2019 at 11:36 AM Alejandro Piñeiro <apinheiro@igalia.com> wrote:
>>> For newcomers to gitlab, it is not evident that it is better to press
>>> the "Resolve Discussion" button when you update your branch handling
>>> feedback.
>>> ---
>>>
>>> As the commit message says, it is not always evident. I was pointed to
>>> do that when I started to use gitlab, and just today I mentioned it to
>>> two different people that didn't know about that.
>>>
>>> Having said so, I feel that the specific text needs some poulishing
>>> first, so any suggestion is welcome.
>>>
>>>   docs/submittingpatches.html | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/docs/submittingpatches.html b/docs/submittingpatches.html
>>> index 020e73d09ec..147b97d76e1 100644
>>> --- a/docs/submittingpatches.html
>>> +++ b/docs/submittingpatches.html
>>> @@ -258,6 +258,10 @@ your email administrator for this.)
>>>   </p>
>>>   <ul>
>>>     <li>Make changes and update your branch based on feedback
>>> +  <li>After an update, for the feedback you handled, close the
>>> +  feedback discussion with the button "Resolve Discussion". In this
>>> +  way the reviewer would know which feedback got handled and which
>>> +  not.
>>>     <li>Old, stale MR may be closed, but you can reopen it if you
>>>       still want to pursue the changes
>>>     <li>You should periodically check to see if your MR needs to be
>>> --
>>> 2.19.1
>>>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev