Screening Tickets (Screeners)
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
To apply someone's changes you need to first create a branch:
$ git checkout -b freds_fixbug42 $ git am --keep-cr -s < 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)
- Please note the -s above, it is important, 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.
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
- 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 aren't sure
- Get a second opinion, and note this in the comments
- Set the waiting-on to Rich or Stu, if appropriate
See Screening Tickets.
Committing Clojure Patches (Committers)