The job of screening is to create a funnel, so that the highest importance, highest urgency items are brought to the BDFL's attention first, already vetted and tested.
Choosing Tickets to Screen
- Bugs first, then features. Most users do not edit the issue type or severity fields, so most tickets start marked as bugs, and job 1 is to recategorize them.
- Focus on tickets with code. If there isn't code yet, you will have to write it, and have someone else screen it.
- No new features during Beta. The most a feature request can hope for during Beta is to be assigned the "Approved Backlog" release
- Do the important bugs first. There is no AI query to find these. One obvious criterion for importance is difficulty of workaround. Another is regression. Another is upvoted issues.
- Vote up issues you think important.
Bounce bad tickets as quickly as possible.
- Tickets are supposed to begin with a discussion, typically on clojure-dev, clojure, the dev wiki, or IRC. Don't sweat this for trivial things like doc spelling fixes, but for anything non-trivial, please mark the ticket as incomplete and request a link to the discussion.
- Decline tickets that do too many things (Too many is usually "more than one."), and ask the submitter to resubmit smaller patches.
- Decline feature enhancements that could begin life as a library instead. Be encouraging and redirect the person to an appropriate Contrib (or propose a new Contrib).
- Decline tickets that are poor style. But remember that Library Coding Standards are guidelines, not fixed laws of the Universe.
- Decline tickets that do not seem comprehensive.
Want to Become a Screener?
Screeners are a subset of contributors who are responsible for moving tickets through a review process, and then funneling tickets to the BDFL.
It would be great to have more screeners. If you are interested, just do all the steps described in this document, without actually changing any ticket attributes. Instead, add a comment to the ticket explaining what you would have done to screen it. (Screeners: keep an eye out for such comments, and where sensible provide feedback so that we are grooming more screeners.)
Process in Detail
Periodically, screeners will review tickets whose approval is marked 'Test'.
- Take ownership of the ticket so that others know you are working on it.
- Make sure to work on defects first. Enhancements are much lower priority, and should typically begin with a design conversation under design
- Read the entire ticket, including linked docs and comments.
To apply someone's changes you need to first create a branch:
$ git checkout -b freds_fixbug42 $ git am --keep-cr -s --ignore-whitespace < their-patch-file.diff
- make sure you have a clean local checkout
- no need to check on multiple OSes (unless the ticket is specific to this)
- If you are committing i.e. you own the lib, please note the importance of -s above, as it indicates that you are the one accepting this patch.
- The --keep-cr helps when files being patched contain DOS CR/LF line endings. It seems to be harmless when it isn't needed, but leave it off or use --no-keep-cr if you suspect it is causing issues.
- The --ignore-whitespace helps when the only changes made to master since the patch was created are to whitespace in the context lines. Without this option, some patches will fail to apply. With that option, screeners can help avoid making contributors update patches merely because some whitespace changed in master.
Once you have a working copy, you should take note of the following kinds of things:
- Review the code in a diff tool to make sure that nothing extraneous snuck in
- Make sure compile and tests pass
- Test the functionality from the REPL
- Does the patch accomplish the goals stated in the ticket (and any accompanying discuss in Confluence/email/IRC)
- If questions were raised in the discussion, are they answered in the ticket?
- Are you happy with the tests, can you follow what they're testing, is there anything missing?
- Are the tests excessive (introducing dependency on implementation detail)?
- Does the documentation still seem right to you?
- Is the implementation idiomatic (if not, explain why, link to examples...)
- Is the patch performance-sensitive? If so, is the approach appropriate?
After review, if you are happy, you should
- Set the approval of the ticket to screened
- If you own the lib in question, you can set approval to accepted, and deploy the change
If you are not happy, but the ticket is fixable:
- Add a comment to the ticket, explaining what the issues are
- Set the approval of the ticket to 'Incomplete'
- Set the waiting-on of the ticket to the person who created the patch
If you are not happy, and the ticket does not seem fixable
- add a comment, explaining the issues
- decline the ticket
If you aren't sure
- Get a second opinion, and note this in the comments
- Set the waiting-on to Rich or Stu, if appropriate