Coordinators for patch review session on Tuesday

Coordinators for patch review session on Tuesday

[Steve George]

Hi all,

The next patch-review session is taking place on Tuesday 2nd of March [0] and I'd love to try pair-programming where groups can actively work on some patch reviews.

Is anyone willing to 'co-ordinate' a pair programming session?

Last time I set-up a cloud host and installed Upterm (https://upterm.dev/) so that everyone could ssh into a session. We could run 4-5 simultaneous sessions where people could 'pair' to do patch reviews together. 

To co-ordinate a session I'll give you SSH access and there are instructions on how to launch the Upterm session. We have written instructions on some basic tools to do the patch reviews - and as Guix is installed for each user you can add your own ;-)

Anyone up for it?

I'm also collecting simple patches for review onto a list so they can be assigned to each group:

https://debbugs.gnu.org/cgi-bin/pkgreport.cgi?tag=patch-review-hackers-list;users=guix

Feel free to add simple issues there!

Thanks,

Futurile / Steve

[0] https://libreplanet.org/wiki/Group:Guix/PatchReviewSessions2024#Patch_Review_Sessions_2024

Re: Coordinators for patch review session on Tuesday

[Christina O'Donnell]

Hi Steve,

I just wanted to say that I enjoyed the first one of these and I'm 
looking forward to today's session. I did want to go to the last 
session, but I lost track of time and missed it!

I'm a new contributor who's only sent a few patches up, but these 
sessions have been helpful for making contributing feel less 
intimidating. I haven't quite felt comfortable enough to review any 
patches on my own, so I do like the idea of pair-reviewing patches with 
someone who's a bit more experienced.

See you this evening!

Kind regards,

Christina

On 31/03/2024 22:58, Steve George wrote:
> Hi all,
>
> The next patch-review session is taking place on Tuesday 2nd of March [0] and I'd love to try pair-programming where groups can actively work on some patch reviews.
>
> Is anyone willing to 'co-ordinate' a pair programming session?
>
> Last time I set-up a cloud host and installed Upterm (https://upterm.dev/) so that everyone could ssh into a session. We could run 4-5 simultaneous sessions where people could 'pair' to do patch reviews together.
>
> To co-ordinate a session I'll give you SSH access and there are instructions on how to launch the Upterm session. We have written instructions on some basic tools to do the patch reviews - and as Guix is installed for each user you can add your own ;-)
>
> Anyone up for it?
>
> I'm also collecting simple patches for review onto a list so they can be assigned to each group:
>
> https://debbugs.gnu.org/cgi-bin/pkgreport.cgi?tag=patch-review-hackers-list;users=guix
>
> Feel free to add simple issues there!
>
> Thanks,
>
> Futurile / Steve
>
> [0] https://libreplanet.org/wiki/Group:Guix/PatchReviewSessions2024#Patch_Review_Sessions_2024
>

Re: Coordinators for patch review session on Tuesday

[Steve George]

On  2 Apr, Christina O'Donnell wrote:
> Hi Steve,
> 
> I just wanted to say that I enjoyed the first one of these and I'm looking
> forward to today's session. I did want to go to the last session, but I lost
> track of time and missed it!
> 
> I'm a new contributor who's only sent a few patches up, but these sessions
> have been helpful for making contributing feel less intimidating. I haven't
> quite felt comfortable enough to review any patches on my own, so I do like
> the idea of pair-reviewing patches with someone who's a bit more
> experienced.
(...)

Hi Christina - thanks for coming along today - I hope it was useful.

There's good instructions on the Wiki on how to review patches:

https://libreplanet.org/wiki?title=Group:Guix/PatchReviewSessions2024

I would love feedback on how to improve them!

There's plenty of patches to review, I've been keeping a list of them for the patch review calls:

https://debbugs.gnu.org/cgi-bin/pkgreport.cgi?tag=patch-review-hackers-list;users=guix

And the wiki page references some other reports.

Please pick some patches and have a go - if you want someone else to look at them feel free to ping here or on IRC!

Thanks,

Steve / Futurile

Re: Coordinators for patch review session on Tuesday

[Christina O'Donnell]

Hi Steve,

On 02/04/2024 21:23, Steve George wrote:
> Hi Christina - thanks for coming along today - I hope it was useful.

Yes I did find it helpful. Since I'm the least experienced out of 
everyone there, I just stayed quiet and tried to absorb as much as I could.

It was good to see that not everyone was using Emacs, and I'm going to 
try to start using Efraim's vi script for GTAGS in Guile.

> There's good instructions on the Wiki on how to review patches:
>
> https://libreplanet.org/wiki?title=Group:Guix/PatchReviewSessions2024
>
> I would love feedback on how to improve them!
>
> There's plenty of patches to review, I've been keeping a list of them for the patch review calls:
>
> https://debbugs.gnu.org/cgi-bin/pkgreport.cgi?tag=patch-review-hackers-list;users=guix
>
> And the wiki page references some other reports.
>
> Please pick some patches and have a go - if you want someone else to look at them feel free to ping here or on IRC!

Thank you for writing this up in so much depth! I've reviewed [1] and 
tried to tag it as reviewed-looks-good, though I don't think that has 
gone through. If you or someone else could take a look at it then I'd 
appreciate that. I plan on reviewing some more patches this evening.

Kind regards,
Christina

[1] https://debbugs.gnu.org/cgi-bin/bugreport.cgi?users=guix;bug=65938

Re: Coordinators for patch review session on Tuesday

[Christina O'Donnell]

Hi,

This is just to say that I went to review [2], but ended up making the 
changes myself, so I've submitted modified patches for those packages. 
Hopefully they're of a quality that's worth pushing.

I'm going to be busy this weekend, but I'll see if I get time to do some 
reviewing later on. It's actually quite fun!

Kind regards,

Christina

[2] https://debbugs.gnu.org/cgi-bin/bugreport.cgi?users=guix;bug=56576

On 03/04/2024 12:00, Christina O'Donnell wrote:
> Hi Steve,
>
> On 02/04/2024 21:23, Steve George wrote:
>> Hi Christina - thanks for coming along today - I hope it was useful.
>
> Yes I did find it helpful. Since I'm the least experienced out of 
> everyone there, I just stayed quiet and tried to absorb as much as I 
> could.
>
> It was good to see that not everyone was using Emacs, and I'm going to 
> try to start using Efraim's vi script for GTAGS in Guile.
>
>> There's good instructions on the Wiki on how to review patches:
>>
>> https://libreplanet.org/wiki?title=Group:Guix/PatchReviewSessions2024
>>
>> I would love feedback on how to improve them!
>>
>> There's plenty of patches to review, I've been keeping a list of them 
>> for the patch review calls:
>>
>> https://debbugs.gnu.org/cgi-bin/pkgreport.cgi?tag=patch-review-hackers-list;users=guix 
>>
>>
>> And the wiki page references some other reports.
>>
>> Please pick some patches and have a go - if you want someone else to 
>> look at them feel free to ping here or on IRC!
>
> Thank you for writing this up in so much depth! I've reviewed [1] and 
> tried to tag it as reviewed-looks-good, though I don't think that has 
> gone through. If you or someone else could take a look at it then I'd 
> appreciate that. I plan on reviewing some more patches this evening.
>
> Kind regards,
> Christina
>
> [1] https://debbugs.gnu.org/cgi-bin/bugreport.cgi?users=guix;bug=65938

Re: Coordinators for patch review session on Tuesday

[Steve George]

Hi,

Comments below:

On 3 Apr, Christina O'Donnell wrote:
(...)
> Thank you for writing this up in so much depth! I've reviewed [1] and tried
> to tag it as reviewed-looks-good, though I don't think that has gone
> through. If you or someone else could take a look at it then I'd appreciate
> that. I plan on reviewing some more patches this evening.
> 
> Kind regards,
> Christina
> 
> [1] https://debbugs.gnu.org/cgi-bin/bugreport.cgi?users=guix;bug=65938
>

1. Changing the tag to reviewed-looks-good

It doesn't look like this worked. The way to do this is in the instructions are 4. 'Set a user tag' [0], probably the easiest way is to send an email (I do get funny results sometimes with my email client):

Subject: setting usertag on 65938

user guix
usertag 65938 + reviewed-looks-good
quit

The first line is important it has to be 'user guix' for it to appear on the patch review reports [1]. I think I messed up the instructions in the Wiki - you have to have a + in between the bug number and the tag you want to set (sorry about that). Please try again.

This is really just a way of signalling that reviews are happening - so trying to keep us in sync. The usertags we're using are:

- patch-review-hackers-list
- under-review
- escalated-review-request 
- waiting-on-contributor
- reviewed-looks-good

The patch changes all look reasonable to me, you've already done a lot:

1. You should add a reviewed-by trailer:
Reviews are contributions from our community (and work!) so we should recognise them and add trailers. It also helps the maintainer know who did the review and therefore the level of confidence.

Basically just add 'Reviewed-by: A Person <a@email.com> - [2]

It looks like your updated patch retriggered QA, so if you look here and the foolow the Data Service link on the right you can see it's building it:

  https://qa.guix.gnu.org/issue/65938

The last step will be for a maintainer to see that it's built correctly, see your review and to apply it - great job for a first patch review!

Steve / Futurile

[0] https://libreplanet.org/wiki/Group:Guix/PatchReviewSessions2024#Patch_review_process_-_CLI_tools
[1] https://libreplanet.org/wiki/Group:Guix/PatchReviewSessions2024#Patch_review_states_and_reports
[2] https://libreplanet.org/wiki/Group:Guix/PatchReviewSessions2024#10._Add_a_Reviewed-by_Trailer

Re: Coordinators for patch review session on Tuesday

[Christina O'Donnell]

Hi,

Thanks for your reply,

> 1. Changing the tag to reviewed-looks-good
>
> It doesn't look like this worked. The way to do this is in the instructions are 4. 'Set a user tag' [0], probably the easiest way is to send an email (I do get funny results sometimes with my email client):
>
> Subject: setting usertag on 65938
>
> user guix
> usertag 65938 + reviewed-looks-good
> quit
>
> The first line is important it has to be 'user guix' for it to appear on the patch review reports [1]. I think I messed up the instructions in the Wiki - you have to have a + in between the bug number and the tag you want to set (sorry about that). Please try again.

Ah I got it this time. I was missing the 'user guix'. I didn't read the 
wiki and tried to look it up from the debbugs documentation.

> This is really just a way of signalling that reviews are happening - so trying to keep us in sync. The usertags we're using are:
>
> - patch-review-hackers-list
> - under-review
> - escalated-review-request
> - waiting-on-contributor
> - reviewed-looks-good
If I change the patch quite a lot, should I mark it as 
'escalated-review-request' instead of 'reviewed-looks-good'?

And should I remove them from the patch-review-hackers-list after I've 
responded
> The patch changes all look reasonable to me, you've already done a lot:
Great, thanks! Good to know I'm doing things vaguely right!
> 1. You should add a reviewed-by trailer:
> Reviews are contributions from our community (and work!) so we should recognise them and add trailers. It also helps the maintainer know who did the review and therefore the level of confidence.
>
> Basically just add 'Reviewed-by: A Person <a@email.com> - [2]
Sure, do you want me resubmit these patches to add that?
> It looks like your updated patch retriggered QA, so if you look here and the foolow the Data Service link on the right you can see it's building it:
>
>    https://qa.guix.gnu.org/issue/65938
>
> The last step will be for a maintainer to see that it's built correctly, see your review and to apply it - great job for a first patch review!
Wonderful! The first of many, I'm hoping.

Kind regards,
Christina

Re: Coordinators for patch review session on Tuesday

[Steve George]

On  4 Apr, Christina O'Donnell wrote:
> Hi,
> 
> Thanks for your reply,
> 
> > 1. Changing the tag to reviewed-looks-good
> > 
> > It doesn't look like this worked. The way to do this is in the instructions are 4. 'Set a user tag' [0], probably the easiest way is to send an email (I do get funny results sometimes with my email client):
> > 
> > Subject: setting usertag on 65938
> > 
> > user guix
> > usertag 65938 + reviewed-looks-good
> > quit
> > 
> > The first line is important it has to be 'user guix' for it to appear on the patch review reports [1]. I think I messed up the instructions in the Wiki - you have to have a + in between the bug number and the tag you want to set (sorry about that). Please try again.
> 
> Ah I got it this time. I was missing the 'user guix'. I didn't read the wiki
> and tried to look it up from the debbugs documentation.
> 
> > This is really just a way of signalling that reviews are happening - so trying to keep us in sync. The usertags we're using are:
> > 
> > - patch-review-hackers-list
> > - under-review
> > - escalated-review-request
> > - waiting-on-contributor
> > - reviewed-looks-good
> If I change the patch quite a lot, should I mark it as
> 'escalated-review-request' instead of 'reviewed-looks-good'?

Since a maintainer will look at it anyway before it's committed, and you are going to 're-roll' it as a v2 (which they will see) I wouldn't bother. I've been using 'escalated-review-request' for (a) ones I see that are just too hard for me, (b) if I personally think there's some outstanding issue with the patch that I can't solve.

> 
> And should I remove them from the patch-review-hackers-list after I've
> responded

Yes please - if you remove it from that list, and you make yourself the owner then I hope we'll keep people co-ordinated!

> > The patch changes all look reasonable to me, you've already done a lot:
> Great, thanks! Good to know I'm doing things vaguely right!
> > 1. You should add a reviewed-by trailer:
> > Reviews are contributions from our community (and work!) so we should recognise them and add trailers. It also helps the maintainer know who did the review and therefore the level of confidence.
> > 
> > Basically just add 'Reviewed-by: A Person <a@email.com> - [2]
> Sure, do you want me resubmit these patches to add that?

No - I think you're all good, it looked they were both going to the build system last night.


> > It looks like your updated patch retriggered QA, so if you look here and the foolow the Data Service link on the right you can see it's building it:
> > 
> >    https://qa.guix.gnu.org/issue/65938
> > 
> > The last step will be for a maintainer to see that it's built correctly, see your review and to apply it - great job for a first patch review!
> Wonderful! The first of many, I'm hoping.
(...)

Keep an eye on them in QA, and see if a maintainer sees them. If nothing happens after 10 days consider pinging on IRC.

Thanks,

Steve