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 account
for. Using KDiff also makes things easier, even
when dealing with formatting changes.
In Emacs, it's also easy to go through every set of
changes, 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 are
intentional, ie just the intended changes only. We
should not submit patches that contain unintended
changes 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 formatting
changes only are
EXTREMELY time consuming, and the patch
attached that issue is a
good example of a painful one. For each of
those line you have to
stare at it looking at every character trying
to figure out what the
point of the bloody change was, or to make sure
that the change is
"invisible"...
That really puzzles me. You keep mentioning
formatting changes when I
don't see any. I didn't make any formatting
changes when modifying the
files. Even now when I look at the patch in Jira,
it shows only the
lines I changed in the file.
*shrug*
As others may have the same question I'll answer
it on the mailing list.
Below is the first section from the
framework_v2.patch file on the
[https://issues.apache.org/jira/browse/OFBIZ-605]
issue.
Each change is marked with a "-" for a line
removed or a "+" for a line
added.
First Set: formatting, or "invisible" change
Second Set: add comment (not necessary, BTW as it
describes something
that no longer exists; if people want to see old
stuff they can look at
the SVN history); remove tabstyles.css link tag
Third Set: formatting, or "invisible" change
Of all of these changes, only the "remove
tabstyles.css link tag" was
actually intended and necessary, but getting to
this fact when reviewing
the changes takes some time... this making it more
difficult to review,
commit, etc.
People tend to slip things into patches
accidentally all the time. I'm
tempted to begun the voting process for a new
policy that says that if
there is anything in the patch file not described
in the summary of the
patch, then it will be rejected...
-David
========================
Index: common/webcommon/includes/header.ftl
===================================================================
--- 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
(page.titleProperty)?has_content>${uiLabelMap[page.titleProperty]}<#else>${(page.title)?if_exists}</#if></title>
- <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 to
maincss.css -->
<link rel="stylesheet"
href="<@ofbizContentUrl>/images/maincss.css</@ofbizContentUrl>"
type="text/css"/>
- <link rel="stylesheet"
href="<@ofbizContentUrl>/images/tabstyles.css</@ofbizContentUrl>"
type="text/css"/>
</#if>
${layoutSettings.extraHead?if_exists}
</head>
@@ -51,7 +51,7 @@
<tr>
<#if
layoutSettings.headerImageUrl?exists>
<td align="left" width="1%"><img
alt="${layoutSettings.companyName}"
src="<@ofbizContentUrl>${layoutSettings.headerImageUrl}</@ofbizContentUrl>"/></td>
- </#if>
+ </#if>
<td align="right" width="1%"
nowrap="nowrap" <#if
layoutSettings.headerRightBackgroundUrl?has_content>background="${layoutSettings.headerRightBackgroundUrl}"</#if>>
<div class="insideHeaderText">
<#if person?has_content>
======================
