I agree. I think it's better that we strongly encourage certain practices (as we are doing now) and bring people to notice when those practices could be improved, but rejection policies put your _means perspective ahead of your _ends. Contributions may be easier to review, but I can gaurantee you with rejection policies, you will then have less of it to review and thereby less solutions making it back into the project.
--- "David E. Jones" <[EMAIL PROTECTED]> wrote: > > 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 > 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 > === message truncated ===
