Uh oh!
There was an error while loading. Please reload this page.
build,windows: clean lint paths in vcbuild.bat - #12278
Conversation
refack
commented
Apr 7, 2017
/cc @nodejs/platform-windows @liusy182 |
refack
commented
Apr 7, 2017
6ef47e1 to
8e66a26Compare* enable eslint to run even in a "clean" workspace * small improvement in performance by reducing number of calls to `findstr` * Document [jslint/jslint-ci] nodejs#11856 (comment)
| :help | ||
| echo vcbuild.bat [debug/release] [msi] [test-all/test-uv/test-inspector/test-internet/test-pummel/test-simple/test-message] [clean] [noprojgen] [small-icu/full-icu/without-intl] [nobuild] [sign] [x86/x64] [vc2015] [download-all] [enable-vtune] [lint/lint-ci] | ||
| echo vcbuild.bat [debug/release] [msi] [test-all/test-uv/test-inspector/test-internet/test-pummel/test-simple/test-message] [clean] [noprojgen] [small-icu/full-icu/without-intl] [nobuild] [sign] [x86/x64] [vc2015] [download-all] [enable-vtune] [lint/lint-ci/jslint/jslint-ci] |
There was a problem hiding this comment.
Is there a reason you're documenting jslint but not cpplint?
There was a problem hiding this comment.
there is no spooncpplint arg, and the jslint arg was missed in #11856 (comment)
There was a problem hiding this comment.
Good point, I didn't notice that there isn't an option to just run cpplint (although there is a cpplint build target). Maybe there should be, there is in the Makefile and it makes sense to have them mirror each other if possible. Doesn't have to be done in this PR though!
| :find_node | ||
| if exist %config%\node set nodeexe=%config%\node&exit /b | ||
| where node > nul 2> nul | ||
| if not errorlevel 1 set nodeexe=node&exit /b |
There was a problem hiding this comment.
This picks up the Node in the path if there isn't a local one right? I guess that's alright for linting, as long as we're not doing it for testing as well.
Could you put a comment in to explain that? Something like:
@rem fall back to node.exe in path, used for linting only.
There was a problem hiding this comment.
I'll rename the var to nodeexe4linting so noone will use it by accidant
| set find_arg=%find_arg% /c:"src\\.*\\.*" | ||
| set find_arg=%find_arg% /c:"test\\addons\\[0-9].*_.*\.h" | ||
| set find_arg=%find_arg% /c:"test\\addons\\[0-9].*_.*\.cc" | ||
| echo %relpath% | findstr %find_arg% > nul |
There was a problem hiding this comment.
Nope. findstr just prints matches, which is kinda useless information
| if exist %config%\node set nodeexe=%config%\node&exit /b | ||
| where node > nul 2> nul | ||
| if not errorlevel 1 set nodeexe=node&exit /b | ||
| echo Could not file node.exe |
I don't really know enough CMD scripting to properly review this, but it generally looks cleaner which is 👍 💯 . Does the CI test linting on windows? I'd assume not. It did fail though, not sure if that's due to this PR. |
refack
commented
Apr 8, 2017
It was like this before see #12276 (comment) |
findstrAfter #12276 I thought the lint code paths deserved some tweaking
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
build,windows