Guido, BTW, next time you propose a patch with new files in it, please don't forget to add the ASL 2 headers in them, thanks.
This of course is true for everybody ;o) Jacques ----- Original Message ----- From: "Guido Amarilla" <[EMAIL PROTECTED]> To: <dev@ofbiz.apache.org> Sent: Tuesday, January 16, 2007 5:58 AM Subject: Re: OFBiz UI work > I´m giving my opinion, because it seems obvious to me that if you > reject a patch, the contributor will correct it in the 99% of the > times...and will learn, so the next time he probably won´t commit the > same mistakes. > This way we reduce the effort to the few commiters increasing the > chances for them to commit other patches.... > > Stronger policies: +1 > > 2007/1/16, David E. Jones <[EMAIL PROTECTED]>: > > > > Ummm... what do you think when you think of a rejection policy? We > > already have patch rejection policies when problems are found... > > > > -David > > > > > > On Jan 15, 2007, at 9:44 PM, Chris Howe wrote: > > > > > 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 === > > > > > > > > > > > > > > -- > Guido Amarilla
