build,tools: implement YAML linting #40007
Conversation
|
@nodejs/linting @Mesteery |
nodejs-github-bot
added
build
|
This doesn't yet address the dreaded "to quote or not to quote strings in YAML" question. I want to get the basic YAML linting implemented and working first before trying to address issues around which there might be conflicting opinions. The things that were flagged by the tool that are fixed in this PR--trailing spaces, inconsistent indentation, and missing newlines at the ends of files--all seem like they would be uncontroversial. |
|
The GitHub Action seems to have worked: https://github.com/nodejs/node/pull/40007/checks?check_run_id=3517929071 |
github-actions
bot
removed
the
request-ci
|
#40004 (comment) |
That sounds like it might be less code inside Node.js core for this, which I would like. How long do you imagine it would take you to create such a thing? It may make sense to land this as something we can do now and move to your tool when it's ready. For that matter, any issues/pain points we find with this solution may inform your design and/or feature set. |
I don't really know, but I think it wouldn't take very long, on the order of a few days/week (if I have the time).
+1 |
|
I don't know what it's worth but I found an ESLint plugin for YAML: |
I'm not opposed to using that instead, but here would be the arguments against it:
|
|
Jenkins linter worked too! (@aduh95 Note the Python 2.7.17. That's from the PYTHON environment variable, which is being overridden by the Jenkins job config to be Python 2 rather than Python 3. 😞 But that can change after 12.x goes EOL, or maybe sooner if we want to mess with it. But I'd want that to be separate from this PR and then we can add the PYTHON env var use back in to this.) https://ci.nodejs.org/job/node-test-linter/42105/console |
|
Oh, wait, darnit, the Jenkins job didn't work: OK, I'll go take a look later and see if I can figure out what went sideways. (Although if anyone beats me to it, that's great!) |
|
I'm not sure we need to wait for Node.js 12.x to go away, AFAIK this job doesn't build I understand that prefer not to do it, and I'm not blocking this PR to be clear. |
I got confused by this in the Jenkins job: So it runs it with Python 3 for Node.js greater than 12.x, but it also runs it with Python 2 for everything regardless of version. BUT....that's just Python linting, not all linting. So I think you're right and I can just use the environment variables. Sorry for the confusion! |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
GitHub actions output:
Pip installing yamllint on Python 3.9.6...
python3 -m pip install --upgrade -t tools/pip/site-packages yamllint || \
python3 -m pip install --upgrade --system -t tools/pip/site-packages yamllint
Collecting yamllint
Downloading yamllint-1.26.3.tar.gz (126 kB)
Collecting pathspec>=0.5.3
Downloading pathspec-0.9.0-py2.py3-none-any.whl (31 kB)
Collecting pyyaml
Downloading PyYAML-5.4.1-cp39-cp39-manylinux1_x86_64.whl (630 kB)
Collecting setuptools
Downloading setuptools-57.5.0-py3-none-any.whl (819 kB)
Using legacy 'setup.py install' for yamllint, since package 'wheel' is not installed.
Installing collected packages: setuptools, pyyaml, pathspec, yamllint
Running setup.py install for yamllint: started
Running setup.py install for yamllint: finished with status 'done'
Successfully installed pathspec-0.9.0 pyyaml-5.4.1 setuptools-57.5.0 yamllint-1.26.3
Makefile:1421: YAML linting with yamllint is not available
Makefile:1421: Run 'make lint-yaml-build'
Fix indentation, traiiling spaces, and missing newline issues in preparation for linting.
@targos Fixed. Thanks. https://github.com/nodejs/node/pull/40007/checks?check_run_id=3523886970 |
VoltrexKeyva
added
author ready
Trott
added
the
commit-queue
github-actions
bot
removed
the
commit-queue
|
Landed in 773a989...b016d5d |
Fix indentation, traiiling spaces, and missing newline issues in preparation for linting. PR-URL: #40007 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>
PR-URL: #40007 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>
PR-URL: #40007 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>
Fix indentation, traiiling spaces, and missing newline issues in preparation for linting. PR-URL: #40007 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>
PR-URL: #40007 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>
PR-URL: #40007 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>
No description provided.