Plots2: Labels and workflow for pull requests to keep up with PR backlog?

[jywarren]

Hi, @publiclab/reviewers -- thanks for all the hard work 🙌 this past week. I'm thinking about how many contributions are coming in (it's been amazing to see, and lots of first-timers-only issues too!) and I was trying to think of how we can streamline our process more so that PRs don't pile up as much.

What do people think? Could we use some more labels, like review-me or add-code-links or something?

Could we add to the Dangerfile that when a PR passes checks, Dangerbot (or the PR template, or another bot) asks the contributor to request a review from @publiclab/reviewers?

Could we have a checklist for reviewing -- indentation/style, tests, good title?

What other ideas do you have?

Thanks everybody!!! 👍

[SidharthBansal]

I will say we should use the "ready" label pr needs to be reviewed. If a person is working on the pr but not yet ready to be viewed he/she can put a "in progress" label. If he/she wants any help then "help-wanted" that way we can easily sort the prs.
Also, about the issues, we can use "wontfix", "help wanted", "discussion" like tags so that if there is help wanted it means the person is stuck and any other member of the community may help him/her.
"discussion" would mean it is either a new feature or something to be added to the core functionality, that needs to be reviewed by @jywarren ,@ebarry for must because the work can't move on without their concern.
Asking people for a review is already a git feature. I can request anyone on plots repo to review my pr. This doesn't needs to be included in the dangerfile in my opinion.
Checklist --we already have tests, we need to add indentation and styles there. Good title is optional because a lot of pr doesn't have to do with title stuff.
Also, indentation is important but I dont know, if we will make use of rubocop for indentation, what will happen to the existing indentation. I have seen many files treated very badly on plots2 in manners of indentation. It may raise errors of those text. But on long run we will have better code.
I will like to suggest one more check mark of "Documentation" as sometimes we write a function and dont write the documentation. Public lab is a growing community and it is of 100+ contributors so sometimes it becomes very difficult to understand what the method is for, to the new developers.
Dangerbot changes will just make the page longer. We already have the feature to request a review on right hand sidebar.

[SidharthBansal]

And contributors have problem whether the work on the issue is going on or not. It can be done by the "assign" feature of github. Contributors may self-assign it if they feel like they can do it else leave it unassigned.
This assign feature will also help the contributors to sort the issues which they are currently solving.

[grvsachdeva]

Hi @jywarren,idea about review-me and add-more-links tags sounds good.The reviewing checklist is a great idea that would benefit us and new contributors too as it will enhance good coding practices and ensure that all necessary steps are taken.
I think relative PR title should be compulsory as it gives an idea about the related issue.
Some Ideas:

1. We should encourage contributors to attach related screenshots and GIF where possible.
2. Documentation about new features should be maintained and highlighted for the site visitors interest boosting.
3. PR's in which contributor feel that it's ready to be merged after fulfilling all the requested changes or for PR in which a small bug is resolved should have a separate tag.

[SidharthBansal]

yeah, PR title is a must.

[SidharthBansal]

One more thing, @jywarren would you mind if contributors may ask the first review from other reviewers and then the final review from you for some Pull Requests. This may decrease your work for certain prs. Like I had the #2213 pr, I know that @sagarpreet-chadha is currently working on it so he can also guide me when you are busy.
This will also add a boost to time saving.

[grvsachdeva]

Nice idea @SidharthBansal .It will surely save time spent on finalizing PR.

[jywarren]

This is all great -- thanks for your thoughts! One thing is, GH doesn't let you self-assign, which is weird, unless you're a project contributor (like @publiclab/reviewers) -- but there are some bots we can install to make that happen.

One thing we could do is make a list of common labels and when to use them -- say, on a page on http://PublicLab.org/wiki/code-labels for example -- and maybe grouped by use, so for reviewers/PRs, vs. for new issues.

https://github.com/publiclab/plots2/labels

[jywarren]

OK awesome -- so just for starters:

Issue labels

Labels help us work through issues faster and more effectively. Help go through pull requests here and label them for faster solving!

• more-detail-please - we need more information before we can start coding on this. Ask @publiclab/community-reps (or something... i can make this group maybe for @ebarry and @steviepubliclab and others who know the broader community well?)
• break-me-up - this could and should be broken into smaller, self-contained projects, for cleaner code separation, more discrete tests, and easier, iterative collaboration
• design - more design work and discussion needed -- i.e. mockups, sketches -- before starting to code
• add-code-links - needs help in finding the exact files and lines of code which would change to solve this issue. Sometimes links to similar 'example' code to use as a model.

Pull request labels

Help go through pull requests here and label them for faster review and adoption!

• review-me - seeking a review from @publiclab/reviewers -- you can also request a review from that group
• ready - ready for final review and merge by @warren
• in-progress - still being worked on.
• needs-help - maybe someone's stuck and/or needs some more information to finish it out!

For pull requests marked with in progress, needs-help, or review-me, here's a checklist of things you can ask for to help a project towards completion:

• [ ] 🎉 encouragement 😄 👍
• [ ] 🔗 links to original issues that they solve with fixes #0000 format in issue body
• [ ] ✍️ more descriptive titles (for quick scanning down a list)
• [ ] 🐞 debugging (if Travis tests did not pass)
• [ ] 👩‍💻 any tips/suggestions on the code itself -- especially simplifying or reducing repetition, increasing readability
• [ ] ✔️ tests to safeguard the new code
• [ ] 📸 screenshots please, if it includes a new design or behavior!

Anything to add here?

[jywarren]

Also, some of this checklist could be made (with friendliness) easier with a Dangerfile script: http://danger.systems/ruby/

Like:

• Warn when library files has been updated but not tests.
• maybe asking for a screenshot if there is a change to /views/?

Cool and also helpful:

# Warn when there is a big PR
warn("Big PR, try to keep changes smaller if you can") if git.lines_of_code > 500

http://danger.systems/reference.html

[ViditChitkara]

Warn when library files has been updated but not tests.
maybe asking for a screenshot if there is a change to /views/?

This is cool. Maybe keeping the lines to 200 would be good. Isn't 500 a bit too much?

[jywarren]

^ agreed, that was just the example!

[ViditChitkara]

:+1:

[SidharthBansal]

This is cool.

[ryzokuken]

Sorry for not being able to catch up with this one, @jywarren. But I was wondering if we could adopt a commit and/or PR naming scheme? That makes things a lot easier, especially when you're parsing stuff visually, say, in a git blame.

For example, a snippet from https://github.com/atom/atom/blob/master/CONTRIBUTING.md:

---

Git Commit Messages

• Use the present tense ("Add feature" not "Added feature")
• Use the imperative mood ("Move cursor to..." not "Moves cursor to...")
• Limit the first line to 72 characters or less
• Reference issues and pull requests liberally after the first line
• When only changing documentation, include [ci skip] in the commit title
• Consider starting the commit message with an applicable emoji:
  
  
  
  • :art: :art: when improving the format/structure of the code
  
  
  • :racehorse: :racehorse: when improving performance
  
  
  • :non-potable_water: :non-potable_water: when plugging memory leaks
  
  
  • :memo: :memo: when writing docs
  
  
  • :penguin: :penguin: when fixing something on Linux
  
  
  • :apple: :apple: when fixing something on macOS
  
  
  • :checkered_flag: :checkered_flag: when fixing something on Windows
  
  
  • :bug: :bug: when fixing a bug
  
  
  • :fire: :fire: when removing code or files
  
  
  • :green_heart: :green_heart: when fixing the CI build
  
  
  • :white_check_mark: :white_check_mark: when adding tests
  
  
  • :lock: :lock: when dealing with security
  
  
  • :arrow_up: :arrow_up: when upgrading dependencies
  
  
  • :arrow_down: :arrow_down: when downgrading dependencies
  
  
  • :shirt: :shirt: when removing linter warnings
  
  

---

We don't even have to necessarily force this scheme upon contributors, the maintainers can just use the squash and merge option and make sure to name commits properly.

[SidharthBansal]

@jywarren can you please disable the feature of adding in progress as soon as one sends a pr?
Many a times it happens that the contributor may want review me or help wanted or add code links?
It will reduce time of the contributors as they can easily tag things according to them
The in-progress tag is only appearing for the reviewers team, I guess

[jywarren]

@SidharthBansal - i have to admit, I'm not sure what's doing that. I don't think I enabled any automated labeling systems, to be honest! I'm a little embarrassed that I don't know what's triggering it. I turned on the GitMate bot for https://github.com/publiclab/image-sequencer/ but not here.

I'll have to look into it because i do think some label automation is helpful, if we can figure out how to configure it!! :-P

I just wanted to say also that I think our labeling "prototype" workflow is working pretty well! I went through and either merged or commented on a bunch today that were ready -- I'm traveling a bit the next few days, and won't have as much time as usual, but I feel like we're still making serious progress!

So, THANK YOU ALL for making this happen! Let's keep this discussion going to identify ways to refine this further, and please chime in with any problems or added suggestions as we go!

[jywarren]

and @ryzokuken I love this idea. When/where would we post such suggestions? Would Dangerbot help with this? Perhaps the PR title is the area we refine it, and then we can copy that title into the squashed commit message?

[ryzokuken]

Sure, that makes sense. I think we can configure Dangerbot to ignore PRs until they're tagged wip. When that tag is removed, they're evaluated and an additional rule is added for the title (which would then be copied by the maintainer to the commit message of the new squashed merge commit).

[seafr]

@jywarren I added the has-pull-request label. This will help avoid multiple PRs for the same issue (such as #2362 & #2363 for #2310).

[SidharthBansal]

Great. I like the idea.

[SidharthBansal]

Moved to #2341