<soapbox> "If a pull request doesn't conform to the declared standard, reject it quickly and move on. "
I understand the spirit of that statement though I see our job of building a community requiring a more accommodating culture both in word and deed. The Robustness Principle is much more appropriate. http://en.wikipedia.org/wiki/Robustness_principle It is frankly an honor for the project that someone would take the time to contribute a PR. Our focus really needs to be on how we find ways to get it accepted. I agree standards are a must. And we should have a high bar. But we should also see ourselves as having a duty to help bring people along. And honestly, it should be fun for people to contribute. Joey E advised me that much of the job of being in the PPMC would be about helping guide PRs across the finish line. He was so right. And if you think about it - that is kind of awesome. </soapbox> On the thread itself: Anyone interested in pushing forward the model/changes to get the formatting process smoothed out please do so. Thanks Joe On Tue, Mar 17, 2015 at 10:17 PM, Adam Taft <[email protected]> wrote: > Ryan, > > Yes, exactly, what you said. Specifically, "ensure the standard is > followed so we can all stop thinking about it." That's exactly my point... > There is no value added dealing with whitespace noise. It shouldn't be > worked around or accommodated. > > If a pull request doesn't conform to the declared standard, reject it > quickly and move on. If a committer merges poorly formatted code, it's on > them to fix it up before pushing to the main branches. Like you said, > declare a standard and stick with it. There may be places that need to be > fixed up once said standard is made, but it should smooth itself out over > time. > > The principle here is minimizing the number of lines that have changed > between two commits, ideally avoiding any file changes that don't add > value, such as whitespace formats, import reordering, etc. This is good > developer etiquette. > > Adam > > > On Tue, Mar 17, 2015 at 8:50 PM, Ryan Blue <[email protected]> wrote: > >> On 03/17/2015 01:35 PM, Adam Taft wrote: >> >>> Whitespace is a holy war and should be avoided. Concern over two space or >>> four space indents, tabs, etc. is not relevant and adds no value (concern >>> with spacing is a form of muda). Diffs and other change evaluations can >>> ideally be done without care for whitespace. This is an easy option for >>> most diff tools. >>> >> >> The important thing is that we set a standard and stick to it. Workflows >> commonly rebase on another branch or merge branches with one another, which >> is a gigantic pain if there are needless conflicts because of whitespace. >> And the bigger issue is leaking non-functional changes. A few aren't a >> terribly big deal, but import reordering and changing indentation are bad. >> >> I'm also thinking about what I do for other projects to maintain a >> consistent version when testing with other projects. In our distribution, >> we snapshot versions and backport changes primarily with cherry-picking. If >> those cherry-picks fail and have huge conflicts all because of whitespace >> or import reordering, we have to do much more work and our confidence is >> lower that we got the backport done right, without bugs. >> >> Sun + 4 space indents seems to be the "norm" for NiFi. I like Sean's >>> second proposal for using jenkins as a nag toward more consistent >>> formatting. There are code formatting options in each IDE to help as >>> well. >>> >> >> +1 to nagging as soon as possible >> >> But ultimately, I don't think a lot of effort should be spent on this. >>> Developers should just be mindful, as with any other type of contribution, >>> that they should conform to the lay of the land. >>> >> >> I agree in principal, but the way to make this happen is to set a standard >> and stick to it. Calling each other on this in code reviews or >> automatically will ensure the standard is followed so we can all stop >> thinking about it. >> >> I personally prefer tabs over spaces for my indents (I could rant why this >>> is "correct" -- again, holy war). But if I'm contributing to NiFi, I'm >>> using 4 space indents, because that's what is normal for the project. >>> >>> Two cents. >>> >>> Adam >>> >>> >>> On Tue, Mar 17, 2015 at 2:29 PM, Sean Busbey <[email protected]> wrote: >>> >>> Set a coding standard (sun + 2 space indents is common), then use >>>> checkstyle to complain when things don't match. It helps when you can >>>> provide IDE formatting configurations that match your chosen coding >>>> standard. >>>> >>> >> For what it's worth, this is what I see on most of the Apache projects >> that I work on. It is slightly more economical if we introduce a rule about >> line maximums. >> >> And speaking of line limits, I think that's one where we don't want to >> have automated enforcement. There's a good argument for it (for vertically >> split editors) but I'd rather a line go a little long because of a change >> than have that change affect the lines around it. Developer discretion >> there. >> >> >> Some of the bigger projects use a jenkins precommit bot that checks (among >>>> other things) that the checkstyle violations don't go up with a proposed >>>> patch. The bot comments on the jira and the committers decide if any >>>> violations need to get fixed. Personally, I like this approach better >>>> than >>>> e.g. trying to fail the build with checkstyle because it allows >>>> incremental >>>> improvement. >>>> >>> >> rb >> >> -- >> Ryan Blue >> Software Engineer >> Cloudera, Inc. >>
