Compile checklist of PR review steps and more clearly share responsibility for readying PRs

[jywarren]

Looking to help check off pull request review steps and get the workflow more clear so we don't get backlogged and more people can help!

@publiclab/reviewers what are our steps?

• add label has-pull-request to the issue having a pr
• link to original issue
• description in title
• reviewed/approved by at least one reviewer
• screenshot if applicable
• passes tests

What else?

• label as ready

[SidharthBansal]

Please check modularity also.

[jywarren]

Yes, so:

• PR is (ideally) less than 50 lines of code & generally tries to implement only a single feature or fix
• PR doesn't affect highly sensitive areas of the site such as header or top of dashboard or signup form without input from @publiclab/community-reps
• Gemfile.lock, yarn.lock, and other unrelated files are not included (message below)

Hi, this is looking great, but would you mind removing the Gemfile.lock from your PR, as I think it's just a version tweak? https://stackoverflow.com/questions/215718/reset-or-revert-a-specific-file-to-a-specific-revision-using-git

How about these couple extra as well? Thanks!!!

[tech4GT]

@jywarren Awesome!! Lot's of nice things in this list!! How about these

• Issue should be claimed before opening the pull request
• Double check if it contains any feature that will be difficult to roll back(Possibly ask for review from other reviewers as well)

[grvsachdeva]

We also have this step in summer:

• label as review-me if review required from another reviewer

[jywarren]

We could start requiring 2 reviews, potentially -- and/or we could mark some people as reviewers who have deeper experience with the site and getting at least one review from this group... what are your thoughts? Thanks, all!

…

On Tue, Oct 2, 2018 at 3:01 PM Gaurav Sachdeva ***@***.***> wrote: We also have this step in summer: - label as review-me if review required from another reviewer — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3563 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJw2sgbia1cIPdEZmm7L6M7gU6A9Bks5ug7fmgaJpZM4XECaC> .

[tech4GT]

@jywarren @SidharthBansal @gauravano I think we should have a project specific team, since not everyone on plots is familiar with Image sequencer and vice versa, apart from the standard reviewers team let's have specific project specific teams with deeper experience with that project, like plots-experts and IS-experts
What say?
Thanks!

[SidharthBansal]

Instead of tagging we can direct Image sequencer to your review only. Leaflet to sagar's review only. Because no one else I assume have knowledge of those projects. After your review jeff can re- review them and merge them.
What you say?

[tech4GT]

@SidharthBansal I think making an experts team still makes sense since in future we may have others who could also get added to that, also there is @ccpandhare who knows the project inside out, and since @jywarren is the only one merging prs anyway, he can review them then. Thanks!

[SidharthBansal]

Actually I have not thought about future. I am really sorry I forget to think about future. Yeah your idea is better. I completely agree with you. Thanks

…

On Wed, Oct 3, 2018, 6:36 PM Varun Gupta ***@***.***> wrote: @SidharthBansal <https://github.com/SidharthBansal> I think making an experts team still makes sense since in future we may have others who could also get added to that, also there is @ccpandhare <https://github.com/ccpandhare> who knows the project inside out, and since @jywarren <https://github.com/jywarren> is the only one merging prs anyway, he can review them then. Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3563 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AUACQ2yC7wMU_d_771P-emXfXv0A1jGEks5uhLZDgaJpZM4XECaC> .

[jywarren]

❤️ this convo. Could we think of a name a little less exclusive sounding than experts? I guess that is what the name reviewers is for, in part. Something else?

[jywarren]

Does everyone know how to use saved replies? It's really helpful to keep a consistent nice tone and provide common links. Sidharth maybe "install saved replies" is itself a task we could make?

[SidharthBansal]

We should just inform this to mentors, there is no need of creating an issue. This depends on individuals, not on community. If we may create an issue then there are potential chances that students will get confused.

…

On Thu, Oct 4, 2018, 1:22 AM Jeffrey Warren ***@***.***> wrote: Does everyone know how to use saved replies? It's really helpful to keep a consistent nice tone and provide common links. Sidharth maybe "install saved replies" is itself a task we could make? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3563 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AUACQ1SnZqz-SlrSOEZwozTZIsyZTY-9ks5uhRWOgaJpZM4XECaC> .

[SidharthBansal]

Hi @jywarren we currently have most of the active members in @publiclab/mentors instead of @publiclab/reviewers. So it would be better to ask from @publiclab/mentors by tagging them. Also, this will be easy for students who are very new to open source.

[SidharthBansal]

We should not use @publiclab/reviewers any more I think so.
We should always use @publiclab/mentors

[tech4GT]

@jywarren how about image-sequencer-specialists ??

[SidharthBansal]

@jywarren can we mark it as gci issue so that gci students can review each other commits and we will have a self-sustained system at Public labs

[SidharthBansal]

This could be multiple instance count (probably infinity) gci issue. If you agree please add gci candidate label.
Thanks

[jywarren]

What about "plots-guides"?

Sidharth, this issue? Maybe we should tidy it up a bit? But I like the idea!!!

[SidharthBansal]

What about "plots-guides"?

@ jywarren Sorry I can't understand what you are referring to.
Yeah, there is a need of making a documentation for this issue.

[jywarren]

Oops, sorry - catching up -- i meant like a group called @publiclab/plots-guides and @publiclab/image-sequencer-guides maybe for people who can guide each project as @tech4GT suggested, and review PRs for each project. So for each pull request we'd mention this group to get a review.

[tech4GT]

Yeah that sounds good to me!

[SidharthBansal]

Hi @jywarren can we add this issue to gci candidate issue?
This way we will have a self sustainable system at pl. Even though we would be busy in some work, students will make issues, solve issues, review PRS. You just need to merge it up. This will save a lot of mentors time too.

[championpaddler]

@SidharthBansal @jywarren We can assign each project to the different group of mentors for reviewing. This way mentors can concentrate on reviewing particular projects PR much better.

[SidharthBansal]

All mentors can review PRS throughout all the repositories of public lab
We have many new mentors who don't know about more than a single repo. Also, it depends on interest of the person, we can't divide people. People have different interests.

[asquare14]

Yes ! I agree with both of you.

[tech4GT]

@SidharthBansal where are we with the teams, did we do it?

[SidharthBansal]

Jeff is out of US. Mentorship form is closed. Applications will be reviewed by 21 october. After he returns US.

[tech4GT]

Oh I meant the guides teams.

[jywarren]

I haven't set them up yet but I can in the next couple days (or today if I get through the PR backlog) -- starting to head back to the US now! Maybe we can start with just a couple repositories to see how it works? Esp. since we can then refer to teams like @publiclab/image-sequencer-guides for repositories that have more specific contributor groups?

Also, for reviewing, did folks see this new GitHub feature for inline suggestions that actually produce a commit? Very cool!

[jywarren]

OK! I created @publiclab/leaflet-environmental-layers-guides and @publiclab/image-sequencer-guides -- let's see how this works and we can expand to more repositories if we find it's useful. Thanks!

[jywarren]

In general, we can make "sub-teams" for different purposes for different repositories. But I'm cautious. We don't want to fragment our community too much, as @SidharthBansal points out. I think we should do our best to have a consistent unified community where people can move easily between projects. So, this is kind of an experiment and we can re-assess how well it works.

[jywarren]

@tech4GT says:

Okay so I am thinking that only the plots-guides and image-sequencer-guides should approve pull requests. This will save us a lot of confusion and prevent incomplete PRS from getting merged.

This sounds reasonable. Can we restrict the specific right to "approve" to a certain team? I haven't seen that exactly. @tech4GT you are an admin in image-sequencer, can you check if this is possible? If so, we could discuss if we want to do this for a bigger project like plots2. Thanks!

[tech4GT]

@jywarren I'll check this out!