Development Team/Code Review - Revision history

Bernie at 02:51, 3 November 2011

2011-11-03T02:51:14Z

← Older revision		Revision as of 22:51, 2 November 2011
Line 46:		Line 46:
	v1->v2: break up _recurse_dir into smaller functions		v1->v2: break up _recurse_dir into smaller functions

	You will first need to set up <code>git send-email</code> so it can send emails through your mail provider. The manual page contains an [http://www.kernel.org/pub/software/scm/git/docs/git-send-email.html#_use_gmail_as_the_smtp_server example for Gmail users]. You can use the [[Service/smtp\|sugarlabs.org SMTP server]] if you have a [[~~Sysadmin~~/~~Shell account request~~\|shell account]]. Feel free to ask for help with setting up <code>git send-email</code> in [[IRC#irc.freenode.net_channels\|#sugar]] or on [[Mailing Lists#Developer Lists\|sugar-devel]].		You will first need to set up <code>git send-email</code> so it can send emails through your mail provider. The manual page contains an [http://www.kernel.org/pub/software/scm/git/docs/git-send-email.html#_use_gmail_as_the_smtp_server example for Gmail users]. You can use the [[Service/smtp\|sugarlabs.org SMTP server]] if you have a [[Service/shell\|shell account]]. Feel free to ask for help with setting up <code>git send-email</code> in [[IRC#irc.freenode.net_channels\|#sugar]] or on [[Mailing Lists#Developer Lists\|sugar-devel]].

	Recommended git user configuration:		Recommended git user configuration:

Quozl: /* Acceptance */

2011-03-22T22:11:11Z

Acceptance

← Older revision		Revision as of 18:11, 22 March 2011
Line 94:		Line 94:

	After your patch has been acknowledged by a maintainer, (s)he will push your code to the repository, including any tags (<code>Reviewed-By:</code>, <code>Tested-By:</code>, etc.) your patch got. From now on your code is part of the Sugar code base and the community (which includes you ;) ) will maintain it for you.		After your patch has been acknowledged by a maintainer, (s)he will push your code to the repository, including any tags (<code>Reviewed-By:</code>, <code>Tested-By:</code>, etc.) your patch got. From now on your code is part of the Sugar code base and the community (which includes you ;) ) will maintain it for you.

			If you are a developer with commit access, a maintainer may ask you to commit it with the tags above. On the other hand, there may be temporal restrictions that the maintainer may want to mention.

	= Dependencies =		= Dependencies =

Quozl: fix page to match erikos policy

2011-03-22T22:05:54Z

fix page to match erikos policy

← Older revision		Revision as of 18:05, 22 March 2011
Line 4:		Line 4:
	Because Sugar is an open source project, anyone can contribute to it and thus improve the learning experience of children all around the world.		Because Sugar is an open source project, anyone can contribute to it and thus improve the learning experience of children all around the world.

	Once you have fixed a bug or implemented a new feature, you ~~can~~ send it to the current maintainers of Sugar by following the code review process.		Once you have fixed a bug or implemented a new feature, you ''must'' send it to the current maintainers of Sugar by following the code review process.

	This process allows the maintainers to feel confident about taking the responsibility of delivering your code to children. This means applying it to the current code base, fixing any bugs that are later discovered in your code or as consequences of it, modifying your code to gain new features, assisting packagers, deployers and translators, etc.		This process allows the maintainers to feel confident about taking the responsibility of delivering your code to children. This means applying it to the current code base, fixing any bugs that are later discovered in your code or as consequences of it, modifying your code to gain new features, assisting packagers, deployers and translators, etc.

Sascha silbe: /* Patch submission */ send-email configuration, add examples

2010-11-28T12:11:11Z

Patch submission: send-email configuration, add examples

← Older revision		Revision as of 08:11, 28 November 2010
Line 45:		Line 45:
	* mention your changes in a changelog below the marker line (the one with just <code>---</code> on it). E.g.:		* mention your changes in a changelog below the marker line (the one with just <code>---</code> on it). E.g.:
	v1->v2: break up _recurse_dir into smaller functions		v1->v2: break up _recurse_dir into smaller functions

			You will first need to set up <code>git send-email</code> so it can send emails through your mail provider. The manual page contains an [http://www.kernel.org/pub/software/scm/git/docs/git-send-email.html#_use_gmail_as_the_smtp_server example for Gmail users]. You can use the [[Service/smtp\|sugarlabs.org SMTP server]] if you have a [[Sysadmin/Shell account request\|shell account]]. Feel free to ask for help with setting up <code>git send-email</code> in [[IRC#irc.freenode.net_channels\|#sugar]] or on [[Mailing Lists#Developer Lists\|sugar-devel]].

	Recommended git user configuration:		Recommended git user configuration:
Line 51:		Line 53:
	* <code>git config --global sendemail.confirm always</code>		* <code>git config --global sendemail.confirm always</code>
	* <code>git config --global 'url.gitorious@git.sugarlabs.org:.pushInsteadOf' git://git.sugarlabs.org/</code>		* <code>git config --global 'url.gitorious@git.sugarlabs.org:.pushInsteadOf' git://git.sugarlabs.org/</code>
			* <code>git config --global alias.send-to-ml-single '!git send-email --stat'</code>
			* <code>git config --global alias.send-to-ml-series '!git send-email --cover-letter --summary --annotate --stat'</code>

			If you agree to publish your changes under the same license as the software you are changing, please add <code>-s</code> to the two aliases above (send-to-ml-single and send-to-ml-series). If you don't agree, don't post the patch! The reason this is mentioned as a separate step instead of adding it to the examples above is so you need to make a conscious decision and can't sue us for copyright infringement.

	Recommended per-repository git configuration, e.g. for sugar-datastore:		Recommended per-repository git configuration, e.g. for sugar-datastore:
Line 61:		Line 67:

	* <code>git config --global merge.conflictstyle diff3</code>		* <code>git config --global merge.conflictstyle diff3</code>
	* <code>git config --global alias.send-to-ml-single '!git send-email -s -p --stat'</code>
	* <code>git config --global alias.send-to-ml-series '!git send-email --cover-letter --summary --annotate -s -p --stat'</code>
	* <code>git config format.headers "Mail-Followup-To: <sugar-devel@lists.sugarlabs.org>"</code>		* <code>git config format.headers "Mail-Followup-To: <sugar-devel@lists.sugarlabs.org>"</code>
	* <code>git config sendemail.envelopesender sascha-ml-sugar-devel@dev.null</code>		* <code>git config sendemail.envelopesender sascha-ml-sugar-devel@dev.null</code>

	~~If you agree to publish your changes under~~ the ~~same license as the software you are changing~~, ~~please add <code>-s</code> to~~ the ~~two~~ aliases ~~above (send-to-ml-single and send-to-ml-series~~). If you don't agree, don't post the patch! The reason this is mentioned as a separate step instead of adding it to the examples above is so you need to make a conscious decision and can't sue us for copyright infringement.		Examples (assuming the above configuration, including the aliases):

			* <code>git send-to-ml-single HEAD^..HEAD</code>
			* after doing the code changes requested during review:
			** <code>git commit -a --amend</code>
			** <code>git send-to-ml-single --annotate --subject-prefix="PATCH v2 sugar"</code>
			*** add the changelog during this step
			*** adjust the subject prefix to use the name of the module you're modifying instead of <code>sugar</code>

	== Discussion ==		== Discussion ==

Sascha silbe: format *-By: tags as


2010-11-28T09:55:53Z
format *-By: tags as <code>

				
				
				
				
				
				← Older revision
				Revision as of 05:55, 28 November 2010
				
Line 75:
Line 75:
Everyone (including you!) is invited to review patches. Not only does this free up time of the maintainers (they can wait until someone else did a first round of review), but also ensures we make the best use of our collective knowledge (no, the maintainers are neither perfect nor omniscient ;) ). Everyone (including you!) is invited to review patches. Not only does this free up time of the maintainers (they can wait until someone else did a first round of review), but also ensures we make the best use of our collective knowledge (no, the maintainers are neither perfect nor omniscient ;) ).



If a reviewer thinks your patch is good (and revelant) enough to be included in the upstream version of the software, (s)he will grant his or her Reviewed-By: tag (see the [http://www.kernel.org/doc/Documentation/SubmittingPatches Linux patch submission process] for details). If a reviewer thinks your patch is good (and revelant) enough to be included in the upstream version of the software, (s)he will grant his or her <code>Reviewed-By:</code> tag (see the [http://www.kernel.org/doc/Documentation/SubmittingPatches Linux patch submission process] for details).



Similarly, everyone is encouraged to try out your patch and give their Tested-By: tag if it worked well for them. Similarly, everyone is encouraged to try out your patch and give their <code>Tested-By:</code> tag if it worked well for them.



After you addressed review comments, you can submit a new version of your patch (see [[#Patch submission|above]] for how to note what you changed). Please make it a reply to your original patch by using the <code>[http://www.kernel.org/pub/software/scm/git/docs/git-format-patch.html#_options --in-reply-to]</code> option to <code>git send-email</code>. E.g.: <code>git send-to-ml-single --in-reply-to=&lt;1290857483.723474@twin.sascha.silbe.org&gt;</code> After you addressed review comments, you can submit a new version of your patch (see [[#Patch submission|above]] for how to note what you changed). Please make it a reply to your original patch by using the <code>[http://www.kernel.org/pub/software/scm/git/docs/git-format-patch.html#_options --in-reply-to]</code> option to <code>git send-email</code>. E.g.: <code>git send-to-ml-single --in-reply-to=&lt;1290857483.723474@twin.sascha.silbe.org&gt;</code>
Line 83:
Line 83:
== Acceptance == == Acceptance ==



After your patch has been acknowledged by a maintainer, (s)he will push your code to the repository, including any tags (Reviewed-By:, Tested-By:, etc.) your patch got.  From now on your code is part of the Sugar code base and the community (which includes you ;) ) will maintain it for you. After your patch has been acknowledged by a maintainer, (s)he will push your code to the repository, including any tags (<code>Reviewed-By:</code>, <code>Tested-By:</code>, etc.) your patch got.  From now on your code is part of the Sugar code base and the community (which includes you ;) ) will maintain it for you.



= Dependencies = = Dependencies =
Line 91:
Line 91:
= Internal process (maintainers only) = = Internal process (maintainers only) =



You can get a ready-to-push version of the patch (including Reviewed-By: etc.) by downloading it in mbox format from [https://patchwork.sugarlabs.org/project/sugar/list/ Patchwork]. You can get a ready-to-push version of the patch (including <code>Reviewed-By:</code> etc.) by downloading it in mbox format from [https://patchwork.sugarlabs.org/project/sugar/list/ Patchwork].



After pushing a patch, please change its state in [https://patchwork.sugarlabs.org/project/sugar/list/ Patchwork] to Accepted + Archived and mark all previous versions (if any are remaining) as Superseded + Archived. If the patch fixes a ticket in [https://bugs.sugarlabs.org/ our bug tracker] please close it, too. After pushing a patch, please change its state in [https://patchwork.sugarlabs.org/project/sugar/list/ Patchwork] to Accepted + Archived and mark all previous versions (if any are remaining) as Superseded + Archived. If the patch fixes a ticket in [https://bugs.sugarlabs.org/ our bug tracker] please close it, too.



[[Category:Development Team]] [[Category:Development Team]]



Sascha silbe: /* Discussion */ mention Tested-By:
2010-11-28T09:54:12Z
Discussion:  mention Tested-By:

				
				
				
				
				
				← Older revision
				Revision as of 05:54, 28 November 2010
				
Line 76:
Line 76:



If a reviewer thinks your patch is good (and revelant) enough to be included in the upstream version of the software, (s)he will grant his or her Reviewed-By: tag (see the [http://www.kernel.org/doc/Documentation/SubmittingPatches Linux patch submission process] for details). If a reviewer thinks your patch is good (and revelant) enough to be included in the upstream version of the software, (s)he will grant his or her Reviewed-By: tag (see the [http://www.kernel.org/doc/Documentation/SubmittingPatches Linux patch submission process] for details).

Similarly, everyone is encouraged to try out your patch and give their Tested-By: tag if it worked well for them.



After you addressed review comments, you can submit a new version of your patch (see [[#Patch submission|above]] for how to note what you changed). Please make it a reply to your original patch by using the <code>[http://www.kernel.org/pub/software/scm/git/docs/git-format-patch.html#_options --in-reply-to]</code> option to <code>git send-email</code>. E.g.: <code>git send-to-ml-single --in-reply-to=&lt;1290857483.723474@twin.sascha.silbe.org&gt;</code> After you addressed review comments, you can submit a new version of your patch (see [[#Patch submission|above]] for how to note what you changed). Please make it a reply to your original patch by using the <code>[http://www.kernel.org/pub/software/scm/git/docs/git-format-patch.html#_options --in-reply-to]</code> option to <code>git send-email</code>. E.g.: <code>git send-to-ml-single --in-reply-to=&lt;1290857483.723474@twin.sascha.silbe.org&gt;</code>



Sascha silbe: add section about maintainer-side process, mentioning Patchwork and Trac
2010-11-27T13:34:10Z
add section about maintainer-side process, mentioning Patchwork and Trac

				
				
				
				
				
				← Older revision
				Revision as of 09:34, 27 November 2010
				
Line 86:
Line 86:



Dependencies need to be fulfilled on all platforms running Sugar (i.e., they need to be available at all and need to be installed which means occupying disk space), so think twice before introducing new ones. Please discuss them on [[Sugar Labs/Communication channels#Developer Lists|sugar-devel]] in advance and clearly state them in the ticket. It's a always a good idea to check whether sufficiently recent versions of your proposed dependencies are already packaged for all (stable releases of the) major distributions, on '''all''' of their (semi-)supported architectures (e.g., ARM and PowerPC). If not, start requesting them (at the distribution bug trackers) ASAP and also mention it on sugar-devel and the ticket. Dependencies need to be fulfilled on all platforms running Sugar (i.e., they need to be available at all and need to be installed which means occupying disk space), so think twice before introducing new ones. Please discuss them on [[Sugar Labs/Communication channels#Developer Lists|sugar-devel]] in advance and clearly state them in the ticket. It's a always a good idea to check whether sufficiently recent versions of your proposed dependencies are already packaged for all (stable releases of the) major distributions, on '''all''' of their (semi-)supported architectures (e.g., ARM and PowerPC). If not, start requesting them (at the distribution bug trackers) ASAP and also mention it on sugar-devel and the ticket.

= Internal process (maintainers only) =

You can get a ready-to-push version of the patch (including Reviewed-By: etc.) by downloading it in mbox format from [https://patchwork.sugarlabs.org/project/sugar/list/ Patchwork].

After pushing a patch, please change its state in [https://patchwork.sugarlabs.org/project/sugar/list/ Patchwork] to Accepted + Archived and mark all previous versions (if any are remaining) as Superseded + Archived. If the patch fixes a ticket in [https://bugs.sugarlabs.org/ our bug tracker] please close it, too.



[[Category:Development Team]] [[Category:Development Team]]



Sascha silbe: document new mailing list based process
2010-11-27T11:59:30Z
document new mailing list based process

				
				
				
				
				
				← Older revision
				Revision as of 07:59, 27 November 2010
				
Line 16:
Line 16:
== Preliminary discussion == == Preliminary discussion ==



Except when the patch will be trivial, it's a good idea to notify the maintainers that you are going to work on this issue and discuss possible approaches with them. This avoids duplicated work and will give you an idea of what is considered a good idea by the maintainers before you commit substantial work in the patch itself. Except when the patch will be trivial, it's a good idea to discuss your idea and possible approaches on [[Mailing Lists#Developer Lists|sugar-devel]]. This avoids duplicated work and will give you an idea of what is considered a good idea by others (maintainers, [[Design Team]], etc.) before you commit substantial work in the patch itself.



May be interesting as well to discuss with the broad community about the usefulness of any proposed new feature. This way the maintainers will feel more motivated to commit their own time so that your work gets included. This is especially important if you're going to make large changes. You will probably be asked to split up your changes in reasonably small, but logically independent parts that can be reviewed somewhat independently.



Plan your work together with the maintainer in such a way that code will be submitted in chunks of reasonable size, a good rule of thumb is in patches of below 2000 lines. 



== Proposal == == Proposal ==



* If your patch addresses an issue which is not registered in [https://bugs.sugarlabs.org the Sugar Labs trac instance], please open a new ticket for it. * If your patch addresses a bug which is not registered on the [https://bugs.sugarlabs.org/ bug tracker] yet, please open a new ticket for it.
* Create your patch using the 'git format-patch' command. (<code>git format-patch HEAD^</code> will create a 00xx-blahblah.patch for every commit. So don't forget to git commit!!) Make sure you mention the ticket number in the commit comment. * Check your code follows the [[Development Team/Code guidelines|code guidelines]]. [[Development Team/Code guidelines#Tools|Automated style checkers]] can be quite helpful.
* Attach the patch to the ticket * Reflect on and explain what your patch does by coming up with a patch description (see [[Activity Team/Git Introduction#Commit message guidelines|commit message guidelines]]).
* Alternatively, you can add a link pointing to a cloned tree. Make sure it contains only the changes you propose for review. * Send your patch to [[Mailing Lists#Developer Lists|sugar-devel]] (see [[#Patch submission|below]] for details).
* Add the keyword 'r?' to call the attention of reviewers. 
* Add a testcase in a ticket comment. You need to mark it with |TestCase|. This adds better readability and our script that pulls together the test cases for each release is able to find it as well. For example: * Add a testcase in a ticket comment. You need to mark it with |TestCase|. This adds better readability and our script that pulls together the test cases for each release is able to find it as well. For example:
  |TestCase|   |TestCase|
Line 34:
Line 32:
  Click on Browse, Read, Pippy icons in the homepage and make sure all of them starts correctly.   Click on Browse, Read, Pippy icons in the homepage and make sure all of them starts correctly.



If your patch is a new feature and reasonably big, you may prefer to submit it for review to the Sugar-devel [http://lists.sugarlabs.org/listinfo/sugar-devel mailing list]. If you do please add the 'r?' keyword to the ticket anyway and reference the mailing list thread. == Patch submission ==



In order to make it easy for the reviewer please: We follow the [http://www.kernel.org/doc/Documentation/SubmittingPatches Linux patch submission process] for the most part. Some points specific to Sugar Labs are:
 
* the affected module (e.g. sugar-toolkit) should be mentioned in the subject prefix
* all patches get sent to [[Mailing Lists#Developer Lists|sugar-devel]]
* you need to clearly note all new dependencies in the patch description (see also [[#Dependencies|below]])
 
If you have revised your patch, addressing review comments, please:
 
* add a version number to the subject prefix (e.g. <code>git send-email --subject-prefix="PATCH v2 sugar"</code>)
* mention your changes in a changelog below the marker line (the one with just <code>---</code> on it). E.g.:
 v1->v2: break up _recurse_dir into smaller functions
 
Recommended git user configuration:
 
* <code>git config --global diff.renames copies</code>
* <code>git config --global sendemail.confirm always</code>
* <code>git config --global 'url.gitorious@git.sugarlabs.org:.pushInsteadOf' git://git.sugarlabs.org/</code>
 
Recommended per-repository git configuration, e.g. for sugar-datastore:
 
* <code>cd ~/sugar-jhbuild/source/sugar-datastore</code>
* <code>git config format.subjectprefix "PATCH sugar-datastore"</code>
* <code>git config sendemail.to "sugar-devel &lt;sugar-devel@lists.sugarlabs.org&gt;"</code>
 
Git configuration options that might come in handy:
 
* <code>git config --global merge.conflictstyle diff3</code>
* <code>git config --global alias.send-to-ml-single '!git send-email -s -p --stat'</code>
* <code>git config --global alias.send-to-ml-series '!git send-email --cover-letter --summary --annotate -s -p --stat'</code>
* <code>git config format.headers "Mail-Followup-To: &lt;sugar-devel@lists.sugarlabs.org&gt;"</code>
* <code>git config sendemail.envelopesender sascha-ml-sugar-devel@dev.null</code>
 
If you agree to publish your changes under the same license as the software you are changing, please add <code>-s</code> to the two aliases above (send-to-ml-single and send-to-ml-series). If you don't agree, don't post the patch! The reason this is mentioned as a separate step instead of adding it to the examples above is so you need to make a conscious decision and can't sue us for copyright infringement.



* note which module is affected e.g., sugar, sugar-toolkit... 
* note possible dependencies e.g. the patch is for sugar but depend on the current HEAD of sugar-toolkit which went in 5 seconds ago (see also separate section about external dependencies below) 



== Discussion == == Discussion ==



Maintainers and peers of the Sugar modules periodically check the review queue in Trac. If a few days pass without comment since the patch was submitted, please ping the maintainer in IRC or in the mailing list. At times maintainers are very busy and will appreciate the ping, even if repeated. Maintainers and other developers will review your patch as their time permits. Please allow for a few days, but feel free to "ping" (by either asking on [[IRC#irc.freenode.net_channels|#sugar]] or replying to your patch) if you haven't received any reply within a week.



The reviewer will read your patch, evaluate its impact to the existing code base and comment on it. The 'r?' keyword will be changed to 'r+' if the patch is accepted, to 'r!' if further information or action is requested from the submitter, or to 'r-' if the patch gets rejected. Everyone (including you!) is invited to review patches. Not only does this free up time of the maintainers (they can wait until someone else did a first round of review), but also ensures we make the best use of our collective knowledge (no, the maintainers are neither perfect nor omniscient ;) ).



See [[Development Team/Code guidelines|Code guidelines]] for common criteria for code in Sugar. If a reviewer thinks your patch is good (and revelant) enough to be included in the upstream version of the software, (s)he will grant his or her Reviewed-By: tag (see the [http://www.kernel.org/doc/Documentation/SubmittingPatches Linux patch submission process] for details).



Once you have answered the questions and/or attached a new patch, set the 'r!' keyword back to 'r?'. After you addressed review comments, you can submit a new version of your patch (see [[#Patch submission|above]] for how to note what you changed). Please make it a reply to your original patch by using the <code>[http://www.kernel.org/pub/software/scm/git/docs/git-format-patch.html#_options --in-reply-to]</code> option to <code>git send-email</code>. E.g.: <code>git send-to-ml-single --in-reply-to=&lt;1290857483.723474@twin.sascha.silbe.org&gt;</code>



== Acceptance == == Acceptance ==



Once your patch has received a 'r+', the reviewer will push your code to the repository. From now on your code is part of the Sugar code base and the community will maintain it for you. After your patch has been acknowledged by a maintainer, (s)he will push your code to the repository, including any tags (Reviewed-By:, Tested-By:, etc.) your patch got.  From now on your code is part of the Sugar code base and the community (which includes you ;) ) will maintain it for you.
  
== Commit == 
If you have commit access to the repositories you should read the [[Activity_Team/Git|commit guidelines]] as well before submitting your changes. 



= Dependencies = = Dependencies =



Tomeu: /* Proposal */
2010-07-14T08:19:15Z
Proposal

				
				
				
				
				
				← Older revision
				Revision as of 04:19, 14 July 2010
				
Line 24:
Line 24:
== Proposal == == Proposal ==



* Create your patch using the 'git format-patch' command. (<code>git format-patch HEAD^</code> will create a 00xx-blahblah.patch for every commit. So don't forget to git commit!!) 
* If your patch addresses an issue which is not registered in [https://bugs.sugarlabs.org the Sugar Labs trac instance], please open a new ticket for it. * If your patch addresses an issue which is not registered in [https://bugs.sugarlabs.org the Sugar Labs trac instance], please open a new ticket for it.
* Create your patch using the 'git format-patch' command. (<code>git format-patch HEAD^</code> will create a 00xx-blahblah.patch for every commit. So don't forget to git commit!!) Make sure you mention the ticket number in the commit comment.
* Attach the patch to the ticket * Attach the patch to the ticket
* Alternatively, you can add a link pointing to a cloned tree. Make sure it contains only the changes you propose for review. * Alternatively, you can add a link pointing to a cloned tree. Make sure it contains only the changes you propose for review.





TimClicks: /* Proposal */ effected -> affected
2010-05-16T21:39:10Z
Proposal:  effected -> affected

				
				
				
				
				
				← Older revision
				Revision as of 17:39, 16 May 2010
				
Line 38:
Line 38:
In order to make it easy for the reviewer please: In order to make it easy for the reviewer please:



* note which module is effected e.g., sugar, sugar-toolkit... * note which module is affected e.g., sugar, sugar-toolkit...
* note possible dependencies e.g. the patch is for sugar but depend on the current HEAD of sugar-toolkit which went in 5 seconds ago (see also separate section about external dependencies below) * note possible dependencies e.g. the patch is for sugar but depend on the current HEAD of sugar-toolkit which went in 5 seconds ago (see also separate section about external dependencies below)

← Older revision		Revision as of 05:55, 28 November 2010
Line 75:		Line 75:
	Everyone (including you!) is invited to review patches. Not only does this free up time of the maintainers (they can wait until someone else did a first round of review), but also ensures we make the best use of our collective knowledge (no, the maintainers are neither perfect nor omniscient ;) ).		Everyone (including you!) is invited to review patches. Not only does this free up time of the maintainers (they can wait until someone else did a first round of review), but also ensures we make the best use of our collective knowledge (no, the maintainers are neither perfect nor omniscient ;) ).

	If a reviewer thinks your patch is good (and revelant) enough to be included in the upstream version of the software, (s)he will grant his or her Reviewed-By: tag (see the [http://www.kernel.org/doc/Documentation/SubmittingPatches Linux patch submission process] for details).		If a reviewer thinks your patch is good (and revelant) enough to be included in the upstream version of the software, (s)he will grant his or her <code>Reviewed-By:</code> tag (see the [http://www.kernel.org/doc/Documentation/SubmittingPatches Linux patch submission process] for details).

	Similarly, everyone is encouraged to try out your patch and give their Tested-By: tag if it worked well for them.		Similarly, everyone is encouraged to try out your patch and give their <code>Tested-By:</code> tag if it worked well for them.

	After you addressed review comments, you can submit a new version of your patch (see [[#Patch submission\|above]] for how to note what you changed). Please make it a reply to your original patch by using the <code>[http://www.kernel.org/pub/software/scm/git/docs/git-format-patch.html#_options --in-reply-to]</code> option to <code>git send-email</code>. E.g.: <code>git send-to-ml-single --in-reply-to=<1290857483.723474@twin.sascha.silbe.org></code>		After you addressed review comments, you can submit a new version of your patch (see [[#Patch submission\|above]] for how to note what you changed). Please make it a reply to your original patch by using the <code>[http://www.kernel.org/pub/software/scm/git/docs/git-format-patch.html#_options --in-reply-to]</code> option to <code>git send-email</code>. E.g.: <code>git send-to-ml-single --in-reply-to=<1290857483.723474@twin.sascha.silbe.org></code>
Line 83:		Line 83:
	== Acceptance ==		== Acceptance ==

	After your patch has been acknowledged by a maintainer, (s)he will push your code to the repository, including any tags (Reviewed-By:, Tested-By:, etc.) your patch got. From now on your code is part of the Sugar code base and the community (which includes you ;) ) will maintain it for you.		After your patch has been acknowledged by a maintainer, (s)he will push your code to the repository, including any tags (<code>Reviewed-By:</code>, <code>Tested-By:</code>, etc.) your patch got. From now on your code is part of the Sugar code base and the community (which includes you ;) ) will maintain it for you.

	= Dependencies =		= Dependencies =
Line 91:		Line 91:
	= Internal process (maintainers only) =		= Internal process (maintainers only) =

	You can get a ready-to-push version of the patch (including Reviewed-By: etc.) by downloading it in mbox format from [https://patchwork.sugarlabs.org/project/sugar/list/ Patchwork].		You can get a ready-to-push version of the patch (including <code>Reviewed-By:</code> etc.) by downloading it in mbox format from [https://patchwork.sugarlabs.org/project/sugar/list/ Patchwork].

	After pushing a patch, please change its state in [https://patchwork.sugarlabs.org/project/sugar/list/ Patchwork] to Accepted + Archived and mark all previous versions (if any are remaining) as Superseded + Archived. If the patch fixes a ticket in [https://bugs.sugarlabs.org/ our bug tracker] please close it, too.		After pushing a patch, please change its state in [https://patchwork.sugarlabs.org/project/sugar/list/ Patchwork] to Accepted + Archived and mark all previous versions (if any are remaining) as Superseded + Archived. If the patch fixes a ticket in [https://bugs.sugarlabs.org/ our bug tracker] please close it, too.

	[[Category:Development Team]]		[[Category:Development Team]]

← Older revision		Revision as of 05:54, 28 November 2010
Line 76:		Line 76:

	If a reviewer thinks your patch is good (and revelant) enough to be included in the upstream version of the software, (s)he will grant his or her Reviewed-By: tag (see the [http://www.kernel.org/doc/Documentation/SubmittingPatches Linux patch submission process] for details).		If a reviewer thinks your patch is good (and revelant) enough to be included in the upstream version of the software, (s)he will grant his or her Reviewed-By: tag (see the [http://www.kernel.org/doc/Documentation/SubmittingPatches Linux patch submission process] for details).

			Similarly, everyone is encouraged to try out your patch and give their Tested-By: tag if it worked well for them.

	After you addressed review comments, you can submit a new version of your patch (see [[#Patch submission\|above]] for how to note what you changed). Please make it a reply to your original patch by using the <code>[http://www.kernel.org/pub/software/scm/git/docs/git-format-patch.html#_options --in-reply-to]</code> option to <code>git send-email</code>. E.g.: <code>git send-to-ml-single --in-reply-to=<1290857483.723474@twin.sascha.silbe.org></code>		After you addressed review comments, you can submit a new version of your patch (see [[#Patch submission\|above]] for how to note what you changed). Please make it a reply to your original patch by using the <code>[http://www.kernel.org/pub/software/scm/git/docs/git-format-patch.html#_options --in-reply-to]</code> option to <code>git send-email</code>. E.g.: <code>git send-to-ml-single --in-reply-to=<1290857483.723474@twin.sascha.silbe.org></code>

← Older revision		Revision as of 09:34, 27 November 2010
Line 86:		Line 86:

	Dependencies need to be fulfilled on all platforms running Sugar (i.e., they need to be available at all and need to be installed which means occupying disk space), so think twice before introducing new ones. Please discuss them on [[Sugar Labs/Communication channels#Developer Lists\|sugar-devel]] in advance and clearly state them in the ticket. It's a always a good idea to check whether sufficiently recent versions of your proposed dependencies are already packaged for all (stable releases of the) major distributions, on '''all''' of their (semi-)supported architectures (e.g., ARM and PowerPC). If not, start requesting them (at the distribution bug trackers) ASAP and also mention it on sugar-devel and the ticket.		Dependencies need to be fulfilled on all platforms running Sugar (i.e., they need to be available at all and need to be installed which means occupying disk space), so think twice before introducing new ones. Please discuss them on [[Sugar Labs/Communication channels#Developer Lists\|sugar-devel]] in advance and clearly state them in the ticket. It's a always a good idea to check whether sufficiently recent versions of your proposed dependencies are already packaged for all (stable releases of the) major distributions, on '''all''' of their (semi-)supported architectures (e.g., ARM and PowerPC). If not, start requesting them (at the distribution bug trackers) ASAP and also mention it on sugar-devel and the ticket.

			= Internal process (maintainers only) =

			You can get a ready-to-push version of the patch (including Reviewed-By: etc.) by downloading it in mbox format from [https://patchwork.sugarlabs.org/project/sugar/list/ Patchwork].

			After pushing a patch, please change its state in [https://patchwork.sugarlabs.org/project/sugar/list/ Patchwork] to Accepted + Archived and mark all previous versions (if any are remaining) as Superseded + Archived. If the patch fixes a ticket in [https://bugs.sugarlabs.org/ our bug tracker] please close it, too.

	[[Category:Development Team]]		[[Category:Development Team]]

← Older revision		Revision as of 07:59, 27 November 2010
Line 16:		Line 16:
	== Preliminary discussion ==		== Preliminary discussion ==

	Except when the patch will be trivial, it's a good idea to ~~notify the maintainers that you are going to work on this issue~~ and ~~discuss~~ possible approaches ~~with them~~. This avoids duplicated work and will give you an idea of what is considered a good idea by ~~the~~ maintainers before you commit substantial work in the patch itself.		Except when the patch will be trivial, it's a good idea to discuss your idea and possible approaches on [[Mailing Lists#Developer Lists\|sugar-devel]]. This avoids duplicated work and will give you an idea of what is considered a good idea by others (maintainers, [[Design Team]], etc.) before you commit substantial work in the patch itself.

	~~May be interesting as well~~ to ~~discuss with the broad community about the usefulness of any proposed new feature~~. ~~This way the maintainers~~ will ~~feel more motivated~~ to ~~commit their own time so~~ that ~~your work gets included~~.		This is especially important if you're going to make large changes. You will probably be asked to split up your changes in reasonably small, but logically independent parts that can be reviewed somewhat independently.

	~~Plan your work together with the maintainer in such a way that code will be submitted in chunks of reasonable size, a good rule of thumb is in patches of below 2000 lines.~~

	== Proposal ==		== Proposal ==

	* If your patch addresses ~~an issue~~ which is not registered in [https://bugs.sugarlabs.org ~~the Sugar Labs trac instance~~], please open a new ticket for it.		* If your patch addresses a bug which is not registered on the [https://bugs.sugarlabs.org/ bug tracker] yet, please open a new ticket for it.
	* ~~Create~~ your ~~patch using~~ the ~~'git format-patch' command~~. ~~(<code>git format-~~patch ~~HEAD^</code> will create~~ a ~~00xx-blahblah.~~patch ~~for every~~ commit~~. So don't forget to git commit!!~~) ~~Make sure you mention the ticket number in the commit comment~~.		* Check your code follows the [[Development Team/Code guidelines\|code guidelines]]. [[Development Team/Code guidelines#Tools\|Automated style checkers]] can be quite helpful.
	* ~~Attach the~~ patch to ~~the ticket~~		* Reflect on and explain what your patch does by coming up with a patch description (see [[Activity Team/Git Introduction#Commit message guidelines\|commit message guidelines]]).
	* Alternatively, you can add a link pointing to a cloned tree. Make sure it contains only the changes you propose for ~~review.~~		* Send your patch to [[Mailing Lists#Developer Lists\|sugar-devel]] (see [[#Patch submission\|below]] for details).
	* Add the keyword 'r?' to call the attention of reviewers.
	* Add a testcase in a ticket comment. You need to mark it with \|TestCase\|. This adds better readability and our script that pulls together the test cases for each release is able to find it as well. For example:		* Add a testcase in a ticket comment. You need to mark it with \|TestCase\|. This adds better readability and our script that pulls together the test cases for each release is able to find it as well. For example:
	\|TestCase\|		\|TestCase\|
Line 34:		Line 32:
	Click on Browse, Read, Pippy icons in the homepage and make sure all of them starts correctly.		Click on Browse, Read, Pippy icons in the homepage and make sure all of them starts correctly.

	If your patch is a new feature and reasonably big, you may prefer to submit it for review to the Sugar-devel [http://lists.sugarlabs.org/listinfo/sugar-devel mailing list]. If you do please add the 'r?' keyword to the ticket anyway and reference the mailing list thread.		== Patch submission ==

	~~In order~~ to ~~make~~ it ~~easy~~ for the ~~reviewer~~ please:		We follow the [http://www.kernel.org/doc/Documentation/SubmittingPatches Linux patch submission process] for the most part. Some points specific to Sugar Labs are:

			* the affected module (e.g. sugar-toolkit) should be mentioned in the subject prefix
			* all patches get sent to [[Mailing Lists#Developer Lists\|sugar-devel]]
			* you need to clearly note all new dependencies in the patch description (see also [[#Dependencies\|below]])

			If you have revised your patch, addressing review comments, please:

			* add a version number to the subject prefix (e.g. <code>git send-email --subject-prefix="PATCH v2 sugar"</code>)
			* mention your changes in a changelog below the marker line (the one with just <code>---</code> on it). E.g.:
			v1->v2: break up _recurse_dir into smaller functions

			Recommended git user configuration:

			* <code>git config --global diff.renames copies</code>
			* <code>git config --global sendemail.confirm always</code>
			* <code>git config --global 'url.gitorious@git.sugarlabs.org:.pushInsteadOf' git://git.sugarlabs.org/</code>

			Recommended per-repository git configuration, e.g. for sugar-datastore:

			* <code>cd ~/sugar-jhbuild/source/sugar-datastore</code>
			* <code>git config format.subjectprefix "PATCH sugar-datastore"</code>
			* <code>git config sendemail.to "sugar-devel <sugar-devel@lists.sugarlabs.org>"</code>

			Git configuration options that might come in handy:

			* <code>git config --global merge.conflictstyle diff3</code>
			* <code>git config --global alias.send-to-ml-single '!git send-email -s -p --stat'</code>
			* <code>git config --global alias.send-to-ml-series '!git send-email --cover-letter --summary --annotate -s -p --stat'</code>
			* <code>git config format.headers "Mail-Followup-To: <sugar-devel@lists.sugarlabs.org>"</code>
			* <code>git config sendemail.envelopesender sascha-ml-sugar-devel@dev.null</code>

			If you agree to publish your changes under the same license as the software you are changing, please add <code>-s</code> to the two aliases above (send-to-ml-single and send-to-ml-series). If you don't agree, don't post the patch! The reason this is mentioned as a separate step instead of adding it to the examples above is so you need to make a conscious decision and can't sue us for copyright infringement.

	* note which module is affected e.g., sugar, sugar-toolkit...
	* note possible dependencies e.g. the patch is for sugar but depend on the current HEAD of sugar-toolkit which went in 5 seconds ago (see also separate section about external dependencies below)

	== Discussion ==		== Discussion ==

	Maintainers and ~~peers of the Sugar modules periodically check the~~ review ~~queue in Trac~~. If a few days ~~pass without comment since the patch was submitted~~, ~~please~~ ping ~~the maintainer in~~ IRC or ~~in the mailing list. At times maintainers are very busy and will appreciate the ping, even~~ if ~~repeated~~.		Maintainers and other developers will review your patch as their time permits. Please allow for a few days, but feel free to "ping" (by either asking on [[IRC#irc.freenode.net_channels\|#sugar]] or replying to your patch) if you haven't received any reply within a week.

	~~The reviewer will read your patch, evaluate its impact~~ to ~~the existing code base and comment on it~~. ~~The 'r?' keyword will be changed to 'r+' if~~ the ~~patch is accepted~~, ~~to 'r!' if further information or action is requested from~~ the ~~submitter~~, ~~or to 'r-' if~~ the ~~patch gets rejected~~.		Everyone (including you!) is invited to review patches. Not only does this free up time of the maintainers (they can wait until someone else did a first round of review), but also ensures we make the best use of our collective knowledge (no, the maintainers are neither perfect nor omniscient ;) ).

	~~See~~ [~~[Development Team~~/~~Code guidelines\|Code guidelines]~~] for ~~common criteria for code in Sugar~~.		If a reviewer thinks your patch is good (and revelant) enough to be included in the upstream version of the software, (s)he will grant his or her Reviewed-By: tag (see the [http://www.kernel.org/doc/Documentation/SubmittingPatches Linux patch submission process] for details).

	~~Once~~ you ~~have answered the questions and/or attached~~ a new patch~~, set~~ the ~~'r!' keyword back~~ to ~~'r?'~~.		After you addressed review comments, you can submit a new version of your patch (see [[#Patch submission\|above]] for how to note what you changed). Please make it a reply to your original patch by using the <code>[http://www.kernel.org/pub/software/scm/git/docs/git-format-patch.html#_options --in-reply-to]</code> option to <code>git send-email</code>. E.g.: <code>git send-to-ml-single --in-reply-to=<1290857483.723474@twin.sascha.silbe.org></code>

	== Acceptance ==		== Acceptance ==

	~~Once~~ your patch has ~~received~~ a ~~'r+'~~, ~~the reviewer~~ will push your code to the repository. From now on your code is part of the Sugar code base and the community will maintain it for you.		After your patch has been acknowledged by a maintainer, (s)he will push your code to the repository, including any tags (Reviewed-By:, Tested-By:, etc.) your patch got. From now on your code is part of the Sugar code base and the community (which includes you ;) ) will maintain it for you.

	~~== Commit ==~~
	~~If you have commit access to the repositories you should read the [[Activity_Team/Git\|commit guidelines]] as well before submitting your changes~~.

	= Dependencies =		= Dependencies =

← Older revision		Revision as of 04:19, 14 July 2010
Line 24:		Line 24:
	== Proposal ==		== Proposal ==

	* Create your patch using the 'git format-patch' command. (<code>git format-patch HEAD^</code> will create a 00xx-blahblah.patch for every commit. So don't forget to git commit!!)
	* If your patch addresses an issue which is not registered in [https://bugs.sugarlabs.org the Sugar Labs trac instance], please open a new ticket for it.		* If your patch addresses an issue which is not registered in [https://bugs.sugarlabs.org the Sugar Labs trac instance], please open a new ticket for it.
			* Create your patch using the 'git format-patch' command. (<code>git format-patch HEAD^</code> will create a 00xx-blahblah.patch for every commit. So don't forget to git commit!!) Make sure you mention the ticket number in the commit comment.
	* Attach the patch to the ticket		* Attach the patch to the ticket
	* Alternatively, you can add a link pointing to a cloned tree. Make sure it contains only the changes you propose for review.		* Alternatively, you can add a link pointing to a cloned tree. Make sure it contains only the changes you propose for review.

← Older revision		Revision as of 17:39, 16 May 2010
Line 38:		Line 38:
	In order to make it easy for the reviewer please:		In order to make it easy for the reviewer please:

	* note which module is ~~effected~~ e.g., sugar, sugar-toolkit...		* note which module is affected e.g., sugar, sugar-toolkit...
	* note possible dependencies e.g. the patch is for sugar but depend on the current HEAD of sugar-toolkit which went in 5 seconds ago (see also separate section about external dependencies below)		* note possible dependencies e.g. the patch is for sugar but depend on the current HEAD of sugar-toolkit which went in 5 seconds ago (see also separate section about external dependencies below)