Meta: bump ecmarkup, enforce linting #1995
Conversation
| @@ -9,7 +9,7 @@ | |||
| "build-spec": "YEAR=2020 && git checkout --quiet \"es${YEAR}\" && mkdir -p \"out/${YEAR}\" && cp -R img \"out/${YEAR}\" && ecmarkup --verbose spec.html \"out/${YEAR}/index.html\" --css \"out/${YEAR}/ecmarkup.css\" --js \"out/${YEAR}/ecmarkup.js\" && git checkout --quiet travis-origin/test-travis", | |||
| "postbuild-spec": "git remote rm travis-origin", | |||
| "prebuild-master": "npm run clean && mkdir out && cp -R img out", | |||
| "build-master": "npm run wipe-es6biblio && ecmarkup --verbose spec.html out/index.html --css out/ecmarkup.css --js out/ecmarkup.js", | |||
| "build-master": "npm run wipe-es6biblio && ecmarkup --lint-spec --verbose spec.html out/index.html --css out/ecmarkup.css --js out/ecmarkup.js", | |||
There was a problem hiding this comment.
rather than add this in build-master, perhaps this should be added as a travis job, so it can run in parallel with the build process?
There was a problem hiding this comment.
The linting isn't really divorced from the building, since they do much of the same processing. And if you pass --lint-spec and there are no linting errors it just builds. (Think of it like -Wall -Werror, if you're familiar with C++ compilers.) I don't think there's much to be gained from splitting it out as a separate job.
There was a problem hiding this comment.
i was thinking clearer error messages - ie, the lint failed but the build passed tells you more than "the build failed"
There was a problem hiding this comment.
@bakkot Thoughts on having a --lint-only that exits after linting, regardless of the number of errors?
There was a problem hiding this comment.
@michaelficarra Some of the things I want to enforce (e.g. the output is valid HTML) could not feasibly done without running the full build, so I'm not sure there would be much point.
There was a problem hiding this comment.
@bakkot That sounds like it could be the job of a separate, more generic tool.
There was a problem hiding this comment.
True.
I'm fine with adding --lint-only, I guess. I am still worried that we may want to add linting checks which are integrated with later steps of the compilation, which would make it less useful.
There was a problem hiding this comment.
@michaelficarra For a concrete example of a late-stage linting error, ecmarkup currently console.logs a warning when it finds a duplicate id or it can't link an xref. It seems like these should be linting errors instead. But building and xref linking happen relatively late in the compilation process.
There was a problem hiding this comment.
It'd still probably be useful to be able to do linting without having to output unwanted artifacts to the filesystem.
bakkot
force-pushed
the
bump-ecmarkup
branch
2 times, most recently
from
d6d67d3 to
92e173f
Compare
|
(force-pushed to update the version of ecmarkup) |
There was a problem hiding this comment.
after looking more into it; i'd have to split up the travis job to have one "install" stage; then a parallel "build" and "lint" stage, and then the "upload" stage; we'll probably want to do that in the future when we try to integrate the Checks API (where we'll want to be able to run ecmarkup and lint without building), but for now, this seems fine
This enforces some simple linting rules for the specification added in tc39/ecmarkup#199. Specifically, during
npm run build-master, it checks:[no LineTerminator here]restrictions or[+flag]gatingIf any lint rule fails, the build fails. Hopefully we'll find out, because there's one place which currently fails, which I'll fix in a second commit.
Note that there's a bunch more things we could reasonably enforce ; PRs (to ecmarkup) welcome!