See inline -----Original Message----- From: Xiang Xiao [mailto:xiaoxiang781...@gmail.com] Sent: Wednesday, March 11, 2020 8:11 AM To: dev@nuttx.apache.org Subject: Re: Should we relax precheck a little bit?
It isn't good to change the licenses in one patch because: 1.We need review all licenses change more carefully, mixing style change make all reviewer lose the focus. [DBS] +1 2.People outside the team may need recheck the licenses change to enuse no any IP pollution. 3.We need find a way to revert the licenses change easily if we do something wrong. [DBS] +1 separate commit per file, 1 or N PRs - NOT Squashed on merge. Finally, it's bad practice to make the unrelated change in one patch, just like you do all operation in a huge function. The best approach is: 1.Change licenses in one patch [DBS] Change licenses 1 commit. 2.Fix the style issue in another patch [DBS] Change style 1 commit. 3.Bug fix or new function in self contained patch [DBS] We also should have a tool to test for binary identity of the builds. This way all the documentation ONLY changes can be verified to be documentation ONLY changes. On Wed, Mar 11, 2020 at 8:59 PM Nathan Hartman <hartman.nat...@gmail.com> wrote: > > On Wed, Mar 11, 2020 at 8:13 AM David Sidrane <david.sidr...@nscdg.com> > wrote: > > > First, I have to say that I value the NuttX coding standard - it make > > for > > the most readable code I have EVERY worked on. I want it preserved. > > > > But in light of the below discussion: I am changing my position that the > > "rule" CI is to just check ONLY the changes to a file AND if the only CI > > failure is a minor style failure the committer may commit it. > > > > To be clear I am not saying that a foreign camel cased contribution is > > allowed in. Just small stuff. > > > > Rational: > > > > Way back, on the list, and in email it was decided that Job #1 was > > having a > > valid style check tool. > > For some it was a foregone conclusion that if CI did style checking the > > tool > > had to 100% accurate and provide go no go results. > > > > If we allocated more time fixing the tool, than reinventing ways to run > > CI, > > we would not be in the state we are in. The message here is: We should > > do a > > better job prioritizing and avoiding reinventing the wheel. > > > > Why avoid the root cause. We need to fix the tool. > > > > Now from a what is required to move forward point of view: We have been > > living under a belief that all the code in the OS 100% conformed to the > > coding standard, and expected that the violations were mostly in arch. > > None > > of this has proven true. The reality of what this means is, that we will > > have to touch every file in the system, (this was expected, but just for > > licenses changes). Running a constantly changing, and sometimes broken > > tool > > constantly on every file in the system is a waste of resources. So we > > need > > to fix the tool _THEN_ fix the code with it. > > > I suggest, until that happens, to do two things in parallel: > > (1) make it the interim policy that *changed* lines have to conform. Over > time this will reduce nonconforming code. > > (2) during the process of fixing licenses, fix also the coding style of > affected files. > > In other words, if changing code, changed lines must conform; if changing > license, make whole file conform. > > Rationale: Commits that make functional change should be limited to just > that functional change and nothing else, for purposes of review, revert, > etc. (License change is not functional change.) > > Cheers > Nathan
