Yes, we want more people, but I don't think anyone wants indiscriminate changes going into SVN! I hate surprises when I check out as much as the next guy, probably more than the next guy in many cases.
Thinking about the next guy is the real key here. You may want to get your patches in ASAP, but if your patch breaks existing code (for example), then that needs to be fixed before the commit is done (either by the committer, the contributor, or an interested third party).
So, yeah, that slows things down and is inconvenient, but keep in mind that a large portion of patches that come into OFBiz break existing functionality. This seems to happen around once a week or so, at least.
It's great that people want to contribute, but in or to contribute to something as large and complex as OFBiz a large amount of effort is necessary, and anyone that wants to help out will err on the side of extra effort, caution and review, and consideration of more general requirements and designs rather just one's own requirements.
-David On Jan 15, 2007, at 7:42 PM, Jonathon -- Improov wrote:
Heh. True. I definitely want MORE people involved in OFBiz.But since I'm not a committer, I'd rather make things easier for the committers. I have no control over how easy or difficult it is to handle patches with "extra unintended footprints".If I were a committer, I would try to automatically filter out formatting changes in my audit, and then INCLUDE the formatting changes. After all, there's no harm removing some extra spaces at the end of lines, part of clean up.We often try to make things easier for the person who is doing a task that we have no control over. Eg, I'd keep my mouth really wide open for my dentist just so his vision is 20/20, no arguments from me; I could afford to be slack about this "mouth wide-open policy" if I were able to do the dentistry myself.But you're certainly right that it'll exclude some people, especially folks who use editors that do not give very much control to users.Jonathon Chris Howe wrote:I don't know that rejection policies are very community friendly. Treating SVN code changes like the keeper of the Bridge of Death (Monty Python Reference, smile) may find less people willing to do in this do-ocracy. Many of us rather like what's left of our anarco-sydicalist commune (oh look, there's another :-) ). --- Jonathon -- Improov <[EMAIL PROTECTED]> wrote:David, I agree with the rejection policy. One suggestion on procedure to take when self-reviewing a patch before submitting it. Look at the diff to see if there are changes we cannot accountfor. Using KDiff also makes things easier, even when dealing with formatting changes.In Emacs, it's also easy to go through every set ofchanges, do an interactive-search for 1st entry of each set, and see if the 2nd entry is highlighted similarly. Meaning, if it is highlighted similarly, the set of changes is bogus(formatting only). In general, we should submit patches that areintentional, ie just the intended changes only. We should not submit patches that contain unintendedchanges that have extra unintended footprints. Jonathon David E. Jones wrote:Moving this back to the mailing list... On Jan 15, 2007, at 5:34 PM, Adrian Crum wrote:David E. Jones wrote:When reviewing a patch lines with formattingchanges only areEXTREMELY time consuming, and the patchattached that issue is agood example of a painful one. For each ofthose line you have tostare at it looking at every character tryingto figure out what thepoint of the bloody change was, or to make surethat the change is"invisible"...That really puzzles me. You keep mentioningformatting changes when Idon't see any. I didn't make any formattingchanges when modifying thefiles. Even now when I look at the patch in Jira,it shows only thelines I changed in the file. *shrug*As others may have the same question I'll answerit on the mailing list.Below is the first section from theframework_v2.patch file on the[https://issues.apache.org/jira/browse/OFBIZ-605]issue.Each change is marked with a "-" for a lineremoved or a "+" for a lineadded. First Set: formatting, or "invisible" change Second Set: add comment (not necessary, BTW as itdescribes somethingthat no longer exists; if people want to see oldstuff they can look atthe SVN history); remove tabstyles.css link tag Third Set: formatting, or "invisible" change Of all of these changes, only the "removetabstyles.css link tag" wasactually intended and necessary, but getting tothis fact when reviewingthe changes takes some time... this making it moredifficult to review,commit, etc. People tend to slip things into patchesaccidentally all the time. I'mtempted to begun the voting process for a newpolicy that says that ifthere is anything in the patch file not describedin the summary of thepatch, then it will be rejected... -David ======================== Index: common/webcommon/includes/header.ftl===================================================================(page.titleProperty)?has_content>${uiLabelMap[page.titleProperty]} <#else>${(page.title)?if_exists}</#if></title>--- common/webcommon/includes/header.ftl(revision 494159)+++ common/webcommon/includes/header.ftl(working copy)@@ -27,7 +27,7 @@ <head> <meta http-equiv="Content-Type"content="text/html; charset=UTF-8"/><title>${layoutSettings.companyName}: <#if- <link rel="shortcut icon"href="<@ofbizContentUrl>/images/ofbiz.ico</@ofbizContentUrl>"/>+ <link rel="shortcut icon"href="<@ofbizContentUrl>/images/ofbiz.ico</@ofbizContentUrl>"/><script language="javascript"src="<@ofbizContentUrl>/images/calendar1.js</@ofbizContentUrl>"type="text/javascript"></script> <script language="javascript"src="<@ofbizContentUrl>/images/selectall.js</@ofbizContentUrl>"type="text/javascript"></script> <script language="javascript"src="<@ofbizContentUrl>/images/fieldlookup.js</@ofbizContentUrl>"type="text/javascript"></script> @@ -37,8 +37,8 @@ <link rel="stylesheet"href="<@ofbizContentUrl>${styleSheet}</@ofbizContentUrl>"type="text/css"/></#list> <#else> + <#-- tabstyles.css has been moved tomaincss.css --><link rel="stylesheet"href="<@ofbizContentUrl>/images/maincss.css</@ofbizContentUrl>"type="text/css"/> - <link rel="stylesheet"href="<@ofbizContentUrl>/images/tabstyles.css</@ofbizContentUrl>"src="<@ofbizContentUrl>${layoutSettings.headerImageUrl}</ @ofbizContentUrl>"/></td>type="text/css"/> </#if> ${layoutSettings.extraHead?if_exists} </head> @@ -51,7 +51,7 @@ <tr> <#iflayoutSettings.headerImageUrl?exists><td align="left" width="1%"><img alt="$ {layoutSettings.companyName}"layoutSettings.headerRightBackgroundUrl?has_content>background="$ {layoutSettings.headerRightBackgroundUrl}"</#if>>- </#if> + </#if> <td align="right" width="1%"nowrap="nowrap" <#if<div class="insideHeaderText"> <#if person?has_content> ======================
smime.p7s
Description: S/MIME cryptographic signature