On Wed, Mar 11, 2020 at 5:43 PM 张铎(Duo Zhang) <palomino...@gmail.com> wrote: > > A possible rule is to not make new nxstyle issues. > You can run nxstyle twice, before the patch and after the patch, and check > the difference to see if there are new violations. > > It may not be accurate but basiclly works, and committers do not need to > check it manually to find out whether the patch introduces new violations. >
Duo, this is the 2nd option I listed before, checkpatch.sh already support to just check the modified lines with -r. > Xiang Xiao <xiaoxiang781...@gmail.com> 于2020年3月11日周三 下午3:42写道： > > > On Wed, Mar 11, 2020 at 2:49 PM Takashi Yamamoto > > <yamam...@midokura.com.invalid> wrote: > > > > > > we basically prevent changes on files unless you can fix all nxstyle > > > issues in them. > > > i don't think it's a good idea. > > > > > > i believe that there can be changes more important than style fixes. > > > in addition to -r option you suggested, > > > i'd suggest to make precheck separate from other (IMO more important) > > > checks like build tests > > > so that reviewers can choose to "ignore" nxstyle errors after inspecting > > them. > > > > > > > Yes, it's possible another solution. I am fine with any approach: > > 1.Must fix nxstyle issue for all whole files > > 2.Must fix nxstyle issue for all modified lines > > 3.Make nxstyle optional, let commiter make the decision. > > The community could discuss with one is best. > > But once the rule is lockdown, we should try our best to follow the > > rule except some special case. > > My bottom line is that the PR must pass all build test before merging. > > > > > On Sun, Mar 8, 2020 at 3:11 AM Xiang Xiao <xiaoxiang781...@gmail.com> > > wrote: > > > > > > > > Hi all, > > > > The precheck ensure the whole file content comform to the coding > > > > style, this strategy has several problems: > > > > 1.Many source file in mainline already violate the coding style > > > > 2.nxstyle frequently generate the false alarm in the current stage > > > > How about we let precheck just ensure the modified line don't violate > > > > the coding standards? > > > > I am asking this question because: > > > > 1.The change in PR 471 has a very good shape: > > > > https://github.com/apache/incubator-nuttx/pull/471/files > > > > but the whole file precheck complain there are many errors: > > > > > > https://github.com/apache/incubator-nuttx/pull/471/checks?check_run_id=492244725 > > > > It is unfair to require the contributor to fix the issue not made > > by them. > > > > 2.Most PR will fail at precheck stage due to item 1 and then block the > > > > more important build test. > > > > > > > > Thanks > > > > Xiang > >
