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
