xiaoxiang781216 commented on issue #2: Master pr nxstyle improvments URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568512309 > > @btashton @davids5 What I mean here is that we don't need modify Makefile, is engouh to add a script invoke nxstyle(maybe build it too) to check the style for specific file/files or commit/commits. > > Then we instruct people integrate this script into pre-commit hook to enable the check automatically. Like: > > https://github.com/torvalds/linux/blob/master/scripts/checkpatch.pl > > Nobody try to integrate checkpatch.pl into Linux build system, since developer can run checkpatch.pl directly to: > > 1.The bunch of specified files > > 2.The bunch of specified commits > > 3.The current living change > > 4.Integrate to pre-commit hook > > > nxstyle(maybe build it too) > > That is make's job. > Yes, but we already have the rule in tools/Makefile.host for generating nxstyle binary, the question is where should we trigger the target. It's better to let the script trigger the buildotherwise user can't invoke the tool outside the make environment(e.g. from jenkins job, precommit hook or command line). > > It's much flexible than make check_format since we can pass the different argument to script form command line, but the same thing is hard to do for Makefile. > > I agree - but adding it to the make file has some benefits and it does not prevent any entity (user, ci, tools) from running the script directly. > > There are a few subtle points here from a non-power users and system POV. > > We want to grow the community it is make for easy of use fot N00Bs > > 1. Most editors and ide's have make integration. > 2. First normal form and abstraction. > > If we add the formatting targets to the `MakeFile` it is top down and use-able by the users, CI and other tools. > > `make check_format` > `make check _format_all` > and for the user (When, we have a decent tool) > `make format` - that fixes the issues > Sorry, I make some confusion here. The build target to check/fix the style issuse is good idea, but we need also consider other use case too(e.g. command line, precommit hook and jenkins job). So it's better to develop the script first and keep all invoking environment in mind and equally(don't bias on make too earily). Once the script is ready, the integrattion is a very simple task. > As of today `nxstyle` lives in `tools` and in tree. It may need to be moved. Think about the best practices with git when the release branch's nxstyle != master's nxstyle and master is correct nxstye (or a conf file for a real tool). > > It will level room for a whole class of errors we can avoid with a simple Workflow. > > Also consider the change to tools like when nxstyle++ is written or we use clang-format. Do you want to update the location and process everywhere: in the CI repos, containers and ./tools or do you want to update the makefile? Yes, I agree we need a shell script here, so we can change the implentation without influencing any user. But we shouldn't move the script to any other location: tools/ is a good place. > > @btashton , @xiaoxiang781216 Please set me straight if I am off course or too myopic. > The things we get wrong now will become technical-debt. Granted at this point it in time it is small but when there are 300 committers on nuttx it will be a problem 300 times lager.
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
