Approval status - the Approval field in a JIRA issue.
Clojure/core member - Contributor who has the approval to vet tickets in JIRA.
Contributor - Person who has signed the Clojure Contributor Agreement.
Defect - A bug in the code base.
Enhancement - A new feature in the code base.
Issue - A ticket in JIRA.
Screener - Contributor who is approved to screen tickets.
Task - Work that must be done outside the code base.
Submitting patches to Clojure and Clojure Contrib
If you've got a bug fix or feature you'd like to submit to the Clojure, Clojure Contrib Libraries, ClojureScript or Clojure CLR projects, first - thanks! Before you get too deep into the code, please make sure you review the contributing guidelines -- it's important to submit a Contributor's agreement as detailed on that page. Once it's been received (your name will appear on the contributor's list on that page), you can join the clojure-dev Google group. Anyone with a CA can become a member of our Confluence and JIRA. Just sign up (please use a recognizable name), then ask on the clojure-dev group and I'll bump up your membership level to make edits.
So You Have an Idea...
In order to make sure you are not duplicating effort, fixing a feature, or planning an unwelcome change, please coordinate with the community, by first examining existing issues (clojure's, links to the various contrib libraries' JIRA projects can be found on the Where Did Clojure Contrib Go page), and discussing your patch plan on the clojure-dev Google group.
Track it in JIRA
If you've got the go-ahead and there isn't already a ticket for your issue/feature, please create one. Make sure the ticket includes a link to the discussion in the Google Group or IRC, so someone reviewing your ticket can quickly get up to speed. Assign yourself to the ticket so people know you are working on it. Please don't assign things to others without their consent.
Prioritization and Milestones (Committers)
Committers use the prioritization and milestone fields. If you are a the committer for a lib, please don't change the priority or the default milestone assignment.
Once you're ready to craft your code, the first thing you'll need is a clone of the Clojure or appropriate repository. The examples below are for the Clojure project -- for submissions to Clojure itself:
Next, create a new branch for yourself:
Now you're ready to get hacking. Make sure relevant doc strings are up to date and that all existing regression tests still pass e.g. for clojure core run:
If you want to add new tests, that would be great too. Once you've finished making your changes you need to commit them.
Now that you've made your changes it's time to get them into a patch. You need to update the repo and fix any conflicts you had.
Once you've fixed any conflicts, you're ready to create a patch:
Now you can attach that patch file to the JIRA ticket. In the More Actions menu near the top of the page, select Attach Files. Please read and follow the recommendations below when writing comments about your attached patch. Screeners have limited time available for screening. You are more likely to get your patch approved if you can be as clear as you can, and as efficient with their time as possible.
- Include the file name and date of the patch in any comments referring to it. It is possible to match up comments with patches based on the date and time, but it is tedious and error prone.
- To get email whenever the ticket is updated, click on the word "Watch" in the top right area of the page. This can help you know when someone else comments on your patch, or creates a new one, etc. Click "Watching" if you want to stop these update emails.
- If you create a new patch that incorporates one or more earlier ones, please combine them all into one patch file, and indicate in your comments that you have done this (with file names and dates of the patches you are superseding).
- If one of your patches becomes superseded by a later one, consider removing your patch to avoid confusion. To do so, go to the page for the ticket and look for the "Attachments" heading beneath the Description text. Far to the right is a plus sign and a triangle. Click on the triangle and select "Manage Attachments" from the menu. Carefully select the one you want to delete, and click the trash can icon next to it.
Mark the ticket as having a patch ready for screening by editing the Patch attribute. Click the Edit button near the top left of the page for the ticket. In the next page find the heading "Patch" with a popup menu next to it. Select "Code" or "Code and Test" from that menu, then click the Update button at the bottom of the page. If you do not see an Edit button on the page for the ticket, and you have signed a CA, ask on the developer's email list to be given permission to edit Jira tickets.
Vetting Tickets (Clojure/core)
The core team vet tickets, getting them into a state where they can be tested, and screened. Steps are:
- Take ownership of a ticket that is status "New" and not currently owned by someone else.
- Verify that the issue is reproducible, adding a test case if necessary.
- Set the approval to "Vetted."
- If the ticket has (or you write) a patch that applies, test it. If it seems to work, set the status to "Test."
- If there isn't a unit test and you feel like writing one, please do so.
- Release ownership of the ticket.
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 Clojure Design
To apply someone's changes you need to first create a branch:
- 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
Committing Clojure Patches (Committers)
- start with a ticket that has been OKed by Rich
- make sure you have the right patch
- make sure the author has signed the CA
- steps that can be batched up:
- double-check that the patch applies cleanly and builds locally
- commit and push the patch
- close the associated ticket(s), if any
I find it safest to do committing from a separate local repository. I have a "clojure" git clone that does not have push permissions for dev and screening, and a separate "clojure-for-commit" checkout for committing. This reduces the chance that my muscle memory will produce a "git push" at the wrong time.
Closing Tickets (Committers)
If you are a committer on a lib, you can screen tickets and close them all in one step. Alternately, you can review tickets screened by others, and then either close them or bounce them back (as in the Screening step above).
While you have the ability to just push your stuff into the repo, please use the tickets systems too. It makes a fine todo system, helps organize bigger milestones, lets others hop in and help or provide input, and provides a place to hang discussions about your changes.
If you have questions or concerns, please ask a question on the clojure-dev Google group.
Clojure is Stricter than Contrib
The expectation of stability and performance is much higher for Clojure than for Clojure Contrib. Rich personally reviews every change to Clojure. To save Rich's time, other committers may do the work in git. When this happens, you can see Rich's review process in JIRA, both in the comments and in tickets being moved from 'Screened' to 'Ok' by Rich.
If you want to share work-in-progress on some feature you are not yet ready to propose with a ticket, you can post a patch made as above, or a pointer to your repo, to the clojure-dev group.
Thanks for contributing to Clojure!
Screening Process Described
An issue is created in JIRA that expresses some unit of work that is desired. This could be a defect, an enhancement or a task.
Clojure/core team vets the ticket, and changes the approval status to Vetted, indicating this is a problem that we want to solve.
Next a contributor writes a patch to address a defect or enhancement, and attaches the patch to the issue. The contributor should also indicate what kind of patch this is in the ticket. The approval status is then set to test, indicating it is dev complete.
A screener will review the issue and marks the approval status as Screened. This will set the issue to waiting on Rich.
Rich comes along and blesses the issue by setting it the approval status to OK, meaning the code is ready to build & push to git.
When the code has been pushed, the issue is marked as Resolved.
At any point along this path a Screener can mark the issue as Declined, meaning it is not something we are going to address.
Also, at any point a Clojure/core member can mark the approval status as Incomplete. This indicates more information is needed to continue work on the issue. There is also an optional Waiting On? field that should be set to indicate who is expected to provide the additional information
Some useful filters
Incomplete Issues: http://dev.clojure.org/jira/secure/IssueNavigator.jspa?mode=hide&requestId=10011
See Also http://clojure.org/patches