Add markdown linting #66
Conversation
There was a problem hiding this comment.
Hi @ntwb Thanks for preparing this.
Please don't take my criticism too much to heart, but to me this feels very over the top with 1300 (!!!) Node packages being installed and a 28K line package.json file, while only 36 packages are actually needed for markdownlint.
The hammer vs screwdriver argument comes to mind.
Also, while I appreciate your "zero-config" intentions, it actually is not. You are just "cheating" and re-using a config defined elsewhere, which we have no control over and no influence on (which kind of makes me uncomfortable).
As for the config which is actually used, this config is highly susceptible to the problems I pointed out earlier in #65 (comment):
markdownlint has a default of
consistentfor a lot of settings, which means it will take the first emphasis marker/bold marker/list marker etc as the "truth" and will demand the rest of the docs follows.
For proper consistent docs, those defaults should be changed as adding a new document, which "happens" to be be scanned first, could otherwise lead to markdownlint demanding all docs to be updated for "consistency".
Other than that:
- Does the
package.jsonfile need to be committed ? Probably a naive question, but this is not a Node project, we are just using a Node tool for QA, so having this committed to the repo would very much misrepresent the project.
We should rename this branch to
trunkto BTW
Why ?
Jest v28 released last week added support for a GitHub Actions Reporter, so once Gutenberg has upgrade to Jest v28 we should be able to also take advantage of line comments reporting from GitHub Actions in the future, it might require a couple of tweaks to be slightly less zero-config
No need to wait for that. You can use the below snippet in the GH Actions workflow to achieve the same:
# @link https://github.com/marketplace/actions/problem-matcher-for-markdownlint-cli
- name: Enable showing issues in PRs
uses: xt0rted/markdownlint-problem-matcher@v1| - name: Setup Cache | ||
| uses: actions/cache@v3 | ||
| with: | ||
| path: ~/.npm | ||
| key: ${{ runner.os }}-node-${{ hashFiles('**/package-lock.json') }} | ||
| restore-keys: | | ||
| ${{ runner.os }}-node- |
There was a problem hiding this comment.
IIRC this should be unnecessary. The setup-node action has caching build in. Any particular reason why the caching from setup-node isn't sufficient ?
There was a problem hiding this comment.
No reason, I thought they were separate so I did it separately
| "devDependencies": { | ||
| "@wordpress/scripts": "^22" | ||
| }, |
There was a problem hiding this comment.
Why are we using this package instead of the markdownlint package ? (also see my remarks about the config in wordpress/scripts)
There was a problem hiding this comment.
I have to agree with Juliette here, the wp scripts package will download 349MB node modules worth of packages for just one package that we need. It seems like an overkill IMO :/
package.json
Outdated
| "bugs": { | ||
| "url": "https://github.com/WordPress/wpcs-docs/issues" | ||
| }, | ||
| "main": "index.js", |
There was a problem hiding this comment.
There is no index.js file in this repo, so what does this refer to ?
| "scripts": { | ||
| "test": "wp-scripts lint-md-docs" | ||
| } |
There was a problem hiding this comment.
To me, this feels very much like obfuscating what's going on. What is wp-scripts ? Why is this called test ? (it's not running tests, it's checking markdown code style) What does the lint-md-docs script(?) do ? And how is it configured ?
Can we please just stick to the necessities instead of pulling in frameworks with overhead we don't need ?
| key: ${{ runner.os }}-node-${{ hashFiles('**/package-lock.json') }} | ||
| restore-keys: | | ||
| ${{ runner.os }}-node- | ||
| - run: npm ci |
There was a problem hiding this comment.
It installs dependencies from the lock file, that way what is installed are known package versions, it prevents unknown package versions from being installed and is faster for CI services than npm install
| "keywords": [ | ||
| "wordpress" | ||
| ], |
There was a problem hiding this comment.
I'm not sure if using the wordpress keyword isn't a bit misleading. Are keywords required ? If not, can we just leave this out ?
There was a problem hiding this comment.
It's from the standard WordPress template for package.json files, so not much thought was given to all this, it will never be published to npmjs.com so is moot for the most part
There was a problem hiding this comment.
In that case, can we strip down the package.json to the essentials ? As everything else is moot anyway.
| "keywords": [ | ||
| "wordpress" | ||
| ], | ||
| "homepage": "https://github.com/WordPress/wpcs-docs#readme", |
There was a problem hiding this comment.
Should this point to the canonical WPCS docs URL in Make ?
There was a problem hiding this comment.
Moot as well, will never be used because the package will never be published,but npm likes to have a somewhat feature-complete file with these fields.
If someone ever wants to run npm home it will open that URL for you though....
|
|
||
| on: | ||
| push: | ||
| branches: [master] |
There was a problem hiding this comment.
Why is branch selection being used ? Feels unnecessary/inhibiting as this action should always run.
Branch selection on push prevents someone from seeing any markdown errors before creating a PR, which generally leads to lots of "Fix code style" commits, which I'd much rather ban altogether.
Branch selection on pull prevents the same for cumulative branches.
There was a problem hiding this comment.
Oh, should we remove this entire section then? This has always been a bit of a mystery to me this branch foo in GA
There was a problem hiding this comment.
Not the entire section, IMO, it should probably read like this:
on:
push:
pull_request:
# Also allow manually triggering the workflow.
workflow_dispatch:
Co-authored-by: Juliette <[email protected]>
|
Regarding the workflow - might also be good to add this (below # Cancels all previous workflow runs for the same branch that have not yet completed.
concurrency:
# The concurrency group contains the workflow name and the branch name.
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: true |
|
Note: I'm drafting a longer comment about a hill I'm willing to die on, I'll share why I chose this approach shortly 😏
Yes, it does, It's a trivial way to have the ability to install packages locally without adding additional tooling, be it good or bad, the majority of WordPress development requires PHP & JS tooling to be installed these days, and I'd like this tooling for this repo to be available to devs locally and not just in GitHub Actions, and this is a quick and easy way to use tooling most WordPress developers would already have installed on their local machine.
This was documented and decided here on make/core some time ago: |
Eagerly waiting to see your arguments. 😇
Sorry, my bad - I meant the The
Ah yes, but this isn't really a code repo and I wonder if renaming the branch wouldn't break the auto-updating of the docs in the manual. |
Nice catch, this will need updating at some point, but GitHub automatically sets up redirects so we should be safe: But we can coordinate this with the meta team and get this change committed at the same time (I can handle that commit, I think) |
Thanks @jrfnl
Thanks @jrfnl, your feedback is always welcome and always appreciated 💯 Jest
Potentially we can look at this change later, but I prefer to use the Jest option I've referenced because of the tight integration into the
|
|
@ntwb I can see your passion in every line of your hill to die on comment and I appreciate you taking the time to write it up (and all your work on the package for which I can definitely see good uses). Unfortunately, your arguments do nothing to convince me that this package is the right choice for this repo.
I would fully understand the choice for using the package as a basis for a (new) JS/CSS project in the WordPress sphere and based on your insights, I would probably even recommend it. However, this project is not a JS/CSS project, which makes most of the arguments fall flat as they are based on an incorrect premise. Secondly, it does nothing to address my comment about the config being unusable. Zero-config is all nice and dandy if there is an actual properly set up config. However, the Not to put too fine a point to it, but the configuration which is proposed to be used is actually pretty crappy and I strongly recommend AGAINST using that config as it will make the markdown unstable and liable to be forced to change based on whichever document is scanned first. Please have a look at the defaults used by markdownlint for everything which is not included in the config and you should see what I mean - especially all those rules which have "consistent" as the default.
That's not a valid argument. The fact that another project does not understand the tooling it is using and is using it incorrectly, does not make a good case for using it here.
We're not talking "some additional bloat", we're talking a massive mamout amount of bloat (1300 packages) versus the mouse we need (36 packages) and the mamout doesn't even come with the configuration we need. To be blunt, I honestly don't care whether it is used by 10 or 10.000.000 other projects. It should only be used when it is the right tool for the job. And in this case I still don't think it is. If nothing else, including this package would now clasify this project as a JS project (due to the MASSIVE
Again, not an argument for using this package. I've (recently) looked at tooling to lint code samples in markdown fenced code blocks before and in my experience those available are extremely limited in their ability and often very buggy, so being able to maybe, potentially use something a few years in the future is no reason to use this package now. We can re-evaluate if and when the time comes this would actually be a viable addition. Also note: that AFAIK there is no linting, nor code style check for fenced codeblocks for PHP available anywhere and while something is better than nothing, that omission is something which I find problematic for the documentation maintained here.
Why wait for something to be added later when a two line addition to the GH Actions script can let us have the same NOW ? I honestly don't understand that argument. |
As promised, pretty much a zero-config markdown linting setup...
I opted for Node.js LTS (currently v16.x) as it should facilitate less maintenance of this in the repo, it should work just fine on Node.js v14 too.
To test:
npm install & npm testornpm i && npm tAlso, I added a basic GitHub Action to run the task on each pull request and merge to
masterbranch, we should rename this branch totrunkto BTW https://github.com/WordPress/wpcs-docs/actions/workflows/lint.ymlJest v28 released last week added support for a GitHub Actions Reporter, so once Gutenberg has upgrade to Jest v28 we should be able to also take advantage of line comments reporting from GitHub Actions in the future, it might require a couple of tweaks to be slightly less zero-config
Until all the outstanding lint issues are fixed, GitHub Actions check will fail, but it's not too much to resolve as #65 covers a lot of these changes