Guidelines for Patch Acceptance/Rejection
Everyone, While there can be no guarantees as to whether a patch will be committed into the Koha codebase, I think in practice there are several requirements. This email is an attempt to identify a few of them, and hopefully start a discussion about whether they are truly requirements, and what others could possibly be added. 1. The patch must do what it claims to do, in all commonly-supported Koha environments 2. The patch must not break existing functionality 3. The patch must apply to the current HEAD of the master branch of the code 4. The patch must follow the Coding Style Guidelines 5. The patch must be MARC-flavour agnostic 6. The patch must contain appropriate copyright information 7. If a database update is require, the patch must handle the update both for new installs and upgrades 8. If a new feature is added, the patch must include appropriate Help documentation What do people think of these requirements? Are they reasonable? Should there be more? I understand that there may not be any set of requirements that's completely sufficient, but if we can identify as many as possible, it would make developers lives a bit easier, since we'd all have a better idea what is needed for our patches to be committable. Cheers, -Ian -- Ian Walls Lead Development Specialist ByWater Solutions Phone # (888) 900-8944 http://bywatersolutions.com ian.walls@bywatersolutions.com Twitter: @sekjal
I have a few extra rules for 3.4 also
From here http://wiki.koha-community.org/wiki/Roadmap_to_3.4
All patches should have at least 1 signoff before the Release Manager looks at them, (exceptions will be made for trivial patches). Preferably 2 signoffs, 1 from the QA manager and 1 from someone else. Although 1 from the QA manager will suffice. All patches should refer to a bug number Chris 2010/11/11 Ian Walls <ian.walls@bywatersolutions.com>:
Everyone,
While there can be no guarantees as to whether a patch will be committed into the Koha codebase, I think in practice there are several requirements. This email is an attempt to identify a few of them, and hopefully start a discussion about whether they are truly requirements, and what others could possibly be added. 1. The patch must do what it claims to do, in all commonly-supported Koha environments 2. The patch must not break existing functionality 3. The patch must apply to the current HEAD of the master branch of the code 4. The patch must follow the Coding Style Guidelines 5. The patch must be MARC-flavour agnostic 6. The patch must contain appropriate copyright information 7. If a database update is require, the patch must handle the update both for new installs and upgrades 8. If a new feature is added, the patch must include appropriate Help documentation What do people think of these requirements? Are they reasonable? Should there be more? I understand that there may not be any set of requirements that's completely sufficient, but if we can identify as many as possible, it would make developers lives a bit easier, since we'd all have a better idea what is needed for our patches to be committable. Cheers,
-Ian -- Ian Walls Lead Development Specialist ByWater Solutions Phone # (888) 900-8944 http://bywatersolutions.com ian.walls@bywatersolutions.com Twitter: @sekjal _______________________________________________ Koha-devel mailing list Koha-devel@lists.koha-community.org http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
And another for 3.2.x: All patches which will not apply cleanly to 3.2.x should be ported and submitted as separate patches marked for 3.2.x. (Hopefully the number of these sort of patches will be few.) Kind Regards, Chris On Wed, Nov 10, 2010 at 6:35 PM, Chris Cormack <chris@bigballofwax.co.nz>wrote:
I have a few extra rules for 3.4 also
From here http://wiki.koha-community.org/wiki/Roadmap_to_3.4
All patches should have at least 1 signoff before the Release Manager looks at them, (exceptions will be made for trivial patches). Preferably 2 signoffs, 1 from the QA manager and 1 from someone else. Although 1 from the QA manager will suffice.
All patches should refer to a bug number
Chris
2010/11/11 Ian Walls <ian.walls@bywatersolutions.com>:
Everyone,
While there can be no guarantees as to whether a patch will be committed into the Koha codebase, I think in practice there are several requirements. This email is an attempt to identify a few of them, and hopefully start a discussion about whether they are truly requirements, and what others could possibly be added. 1. The patch must do what it claims to do, in all commonly-supported Koha environments 2. The patch must not break existing functionality 3. The patch must apply to the current HEAD of the master branch of the code 4. The patch must follow the Coding Style Guidelines 5. The patch must be MARC-flavour agnostic 6. The patch must contain appropriate copyright information 7. If a database update is require, the patch must handle the update both for new installs and upgrades 8. If a new feature is added, the patch must include appropriate Help documentation What do people think of these requirements? Are they reasonable? Should there be more? I understand that there may not be any set of requirements that's completely sufficient, but if we can identify as many as possible, it would make developers lives a bit easier, since we'd all have a better idea what is needed for our patches to be committable. Cheers,
-Ian -- Ian Walls Lead Development Specialist ByWater Solutions Phone # (888) 900-8944 http://bywatersolutions.com ian.walls@bywatersolutions.com Twitter: @sekjal _______________________________________________ Koha-devel mailing list Koha-devel@lists.koha-community.org http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
_______________________________________________ Koha-devel mailing list Koha-devel@lists.koha-community.org http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
2010/11/10 Chris Cormack <chris@bigballofwax.co.nz>
I have a few extra rules for 3.4 also
From here http://wiki.koha-community.org/wiki/Roadmap_to_3.4
All patches should have at least 1 signoff before the Release Manager looks at them, (exceptions will be made for trivial patches). Preferably 2 signoffs, 1 from the QA manager and 1 from someone else. Although 1 from the QA manager will suffice.
All patches should refer to a bug number
Chris
2010/11/11 Ian Walls <ian.walls@bywatersolutions.com>:
Everyone,
While there can be no guarantees as to whether a patch will be committed into the Koha codebase, I think in practice there are several requirements. This email is an attempt to identify a few of them, and hopefully start a discussion about whether they are truly requirements, and what others could possibly be added. 1. The patch must do what it claims to do, in all commonly-supported Koha environments 2. The patch must not break existing functionality 3. The patch must apply to the current HEAD of the master branch of the code 4. The patch must follow the Coding Style Guidelines 5. The patch must be MARC-flavour agnostic 6. The patch must contain appropriate copyright information 7. If a database update is require, the patch must handle the update both for new installs and upgrades 8. If a new feature is added, the patch must include appropriate Help documentation What do people think of these requirements? Are they reasonable? Should there be more? I understand that there may not be any set of requirements that's completely sufficient, but if we can identify as many as possible, it would make developers lives a bit easier, since we'd all have a better idea what is needed for our patches to be committable. Cheers,
-Ian -- Ian Walls Lead Development Specialist ByWater Solutions Phone # (888) 900-8944 http://bywatersolutions.com ian.walls@bywatersolutions.com Twitter: @sekjal _______________________________________________ Koha-devel mailing list Koha-devel@lists.koha-community.org http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
_______________________________________________ Koha-devel mailing list Koha-devel@lists.koha-community.org http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
Question for Mr. 3.4 RM: Is the procedure for dealing with DB revision numbers still the same? As far as I remember from the 3.2 development days, the procedure was to patch kohastructure.sql (or sysprefs.sql, or whatever), then add the update to the end of updatedatabase.pl with a generic version number, like 3.01.00.XXX. Patching kohastructure.pl was left to the RM when they applied the patch. I had a crazy table on the wiki for a bit, but this seemed to work better. That still the consensus? -- Jesse Weaver
Question for Mr. 3.4 RM:
:)
Is the procedure for dealing with DB revision numbers still the same? As far as I remember from the 3.2 development days, the procedure was to patch kohastructure.sql (or sysprefs.sql, or whatever), then add the update to the end of updatedatabase.pl with a generic version number, like 3.01.00.XXX. Patching kohastructure.pl was left to the RM when they applied the patch.
Patching kohaversion.pl you mean?
I had a crazy table on the wiki for a bit, but this seemed to work better.
That still the consensus?
Yup that is the current practice. If we do implement DBIx::Class::Schema and DBIx::Class::Schema::Versioned, updatedatabase.pl and kohastructure.pl might both go away. But not yet. Chris
It is worth stressing that there are things which will encourage quick adoption of a patch. Good commit messages, tests that prove it does what it says and doesn't add needlessly to the amount of entropy in the universe. -- Colin Campbell Chief Software Engineer, PTFS Europe Limited Content Management and Library Solutions +44 (0) 208 366 1295 (phone) +44 (0) 7759 633626 (mobile) colin.campbell@ptfs-europe.com skype: colin_campbell2 http://www.ptfs-europe.com
Hi, On Thu, Nov 11, 2010 at 4:11 AM, Colin Campbell <colin.campbell@ptfs-europe.com> wrote:
It is worth stressing that there are things which will encourage quick adoption of a patch. Good commit messages, tests that prove it does what it says and doesn't add needlessly to the amount of entropy in the universe.
Here are other characteristics of a good patch: [1] The patch covers a single topic. For example, a patch that fixes a circulation bug and adds a new cataloging feature is problematic; if the circ bugfix works but the new cataloging feature doesn't, the patch can't be accepted as is. Also, such a patch is more difficult to test; somebody who is an expert in Koha's circulation module may not be comfortable signing off on a cataloging enhancement. [2] The patch is easy to read. In particular, please don't mix up major whitespace correction and functionality. If you want to clean up whitespace, please do so in a separate patch. [3] The patch does not contain the detritus of your coding process. In other words, the patch shouldn't contain things like unguarded warns left over from your debugging or temporary files. Regards, Galen -- Galen Charlton gmcharlt@gmail.com
I have compiled these guidelines into a wiki page: http://wiki.koha-community.org/wiki/Guidelines_for_Patch_Acceptance/Rejectio.... If others would like to weigh in, please feel free to update the page in the wiki. Cheers, -Ian On Thu, Nov 11, 2010 at 7:50 AM, Galen Charlton <gmcharlt@gmail.com> wrote:
Hi,
On Thu, Nov 11, 2010 at 4:11 AM, Colin Campbell <colin.campbell@ptfs-europe.com> wrote:
It is worth stressing that there are things which will encourage quick adoption of a patch. Good commit messages, tests that prove it does what it says and doesn't add needlessly to the amount of entropy in the universe.
Here are other characteristics of a good patch:
[1] The patch covers a single topic.
For example, a patch that fixes a circulation bug and adds a new cataloging feature is problematic; if the circ bugfix works but the new cataloging feature doesn't, the patch can't be accepted as is. Also, such a patch is more difficult to test; somebody who is an expert in Koha's circulation module may not be comfortable signing off on a cataloging enhancement.
[2] The patch is easy to read.
In particular, please don't mix up major whitespace correction and functionality. If you want to clean up whitespace, please do so in a separate patch.
[3] The patch does not contain the detritus of your coding process.
In other words, the patch shouldn't contain things like unguarded warns left over from your debugging or temporary files.
Regards,
Galen -- Galen Charlton gmcharlt@gmail.com _______________________________________________ Koha-devel mailing list Koha-devel@lists.koha-community.org http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
-- Ian Walls Lead Development Specialist ByWater Solutions Phone # (888) 900-8944 http://bywatersolutions.com ian.walls@bywatersolutions.com Twitter: @sekjal
Le 11/11/2010 04:07, Chris Cormack a écrit :
Question for Mr. 3.4 RM:
:)
Is the procedure for dealing with DB revision numbers still the same? As far as I remember from the 3.2 development days, the procedure was to patch kohastructure.sql (or sysprefs.sql, or whatever), then add the update to the end of updatedatabase.pl with a generic version number, like 3.01.00.XXX. Patching kohastructure.pl was left to the RM when they applied the patch.
Patching kohaversion.pl you mean?
I had a crazy table on the wiki for a bit, but this seemed to work better.
That still the consensus?
Yup that is the current practice.
If we do implement DBIx::Class::Schema and DBIx::Class::Schema::Versioned, updatedatabase.pl and kohastructure.pl might both go away. But not yet. Well, as far as DB structure is concerned, this is ok. But if we need some new systempreference or some new data in the database (for instance some change in the marc framework...) then Schema and its versioning system would not be enough. Any plans for that ? -- Henri-Damien LAURENT
On 15 November 2010 08:41, LAURENT Henri-Damien <henridamien.laurent@biblibre.com> wrote:
Le 11/11/2010 04:07, Chris Cormack a écrit :
Question for Mr. 3.4 RM:
:)
Is the procedure for dealing with DB revision numbers still the same? As far as I remember from the 3.2 development days, the procedure was to patch kohastructure.sql (or sysprefs.sql, or whatever), then add the update to the end of updatedatabase.pl with a generic version number, like 3.01.00.XXX. Patching kohastructure.pl was left to the RM when they applied the patch.
Patching kohaversion.pl you mean?
I had a crazy table on the wiki for a bit, but this seemed to work better.
That still the consensus?
Yup that is the current practice.
If we do implement DBIx::Class::Schema and DBIx::Class::Schema::Versioned, updatedatabase.pl and kohastructure.pl might both go away. But not yet. Well, as far as DB structure is concerned, this is ok. But if we need some new systempreference or some new data in the database (for instance some change in the marc framework...) then Schema and its versioning system would not be enough. Any plans for that ?
Thats a good question, one I don't have a good answer for yet, but I do think your atomic updates work is certainly a step in the right direction. ANSI compliant sql inserts/updates for the win :) Chris
Le 14/11/2010 20:45, Chris Cormack a écrit :
On 15 November 2010 08:41, LAURENT Henri-Damien <henridamien.laurent@biblibre.com> wrote:
Le 11/11/2010 04:07, Chris Cormack a écrit :
Question for Mr. 3.4 RM:
:)
Is the procedure for dealing with DB revision numbers still the same? As far as I remember from the 3.2 development days, the procedure was to patch kohastructure.sql (or sysprefs.sql, or whatever), then add the update to the end of updatedatabase.pl with a generic version number, like 3.01.00.XXX. Patching kohastructure.pl was left to the RM when they applied the patch.
Patching kohaversion.pl you mean?
I had a crazy table on the wiki for a bit, but this seemed to work better.
That still the consensus?
Yup that is the current practice.
If we do implement DBIx::Class::Schema and DBIx::Class::Schema::Versioned, updatedatabase.pl and kohastructure.pl might both go away. But not yet. Well, as far as DB structure is concerned, this is ok. But if we need some new systempreference or some new data in the database (for instance some change in the marc framework...) then Schema and its versioning system would not be enough. Any plans for that ?
Thats a good question, one I don't have a good answer for yet, but I do think your atomic updates work is certainly a step in the right direction. Would need some concensus on the directory structure, which could help. Why not a directory structure db_updates |- acquisitions |- members/patrons/borrowers |- ...... One file for each update. And have a directory by release and symlinks to those db_updates. .... But there was a discussion on that... http://lists.koha.org/pipermail/koha-devel/2010-February/033588.html No concensus reached... No decisions, no collaboration planned. But that would be quite a change. for all the previous works... could be quite tedious.
ANSI compliant sql inserts/updates for the win :) -- Henri-Damien LAURENT
Hi, I have a request about "Guidelines for Patch Acceptance/Rejection". In specific about "How do we send the patches to koha-patches@lists.koha-community.org ? Now I work on Windows 7 with GitExtensions ver 2.25 <http://code.google.com/p/gitextensions/> To send a mail with a patch i need to use the command 'format patch' The result is here: http://lists.koha-community.org/pipermail/koha-patches/2010-November/013015.... In fact the patch is in the attach file, http://lists.koha-community.org/pipermail/koha-patches/attachments/20101115/... Do you think is a correct way to send a patch ? Bye Zeno Tajoli -- Zeno Tajoli CILEA - Segrate (MI) tajoliAT_SPAM_no_prendiATcilea.it (Indirizzo mascherato anti-spam; sostituisci qaunto tra AT con @)
Hi, On Mon, Nov 15, 2010 at 10:51 AM, Zeno Tajoli <tajoli@cilea.it> wrote:
In fact the patch is in the attach file, http://lists.koha-community.org/pipermail/koha-patches/attachments/20101115/...
Do you think is a correct way to send a patch ?
I've found it slightly quicker to process patch emails when the patch is in the body of the message (e.g., as generated by the command-line git format-patch), so personally I prefer that where possible. However, since there are cases where that won't work (e.g., because the patch contains a line which is too long, which git send-email won't accept, or because the contributor may have a MTA setup at work that doesn't readily support mailing patches directly with git send-email), submitting the patches as attachments is also acceptable. Pull requests from a public Git repository are also fine. Regards, Galen -- Galen Charlton gmcharlt@gmail.com
participants (8)
-
Chris Cormack -
Chris Nighswonger -
Colin Campbell -
Galen Charlton -
Ian Walls -
Jesse -
LAURENT Henri-Damien -
Zeno Tajoli