🏗✨ Add Markdown linting #3346
🏗✨ Add Markdown linting #3346
Conversation
|
Thanks, @DerekNonGeneric! Can you quickly summarise what effect running this would have on the current set of documents? That would also help deciding between total conformance vs incremental adoption 😉 If we then adopt this change then I think it should run as part of all other checks both in a commit hook as well as during prebuild phase. Will remark-validate-links make our custom logic in GrowReferenceChecker.js obsolete? Something that actually is a real problem besides style is the following: we have some custom Markdown tags. |
|
+1 to Matthias's comments. Another thing: we should use the same markdown formatting setup as ampproject/amphtml (I think they use prettier for this). |
|
/to @matthiasrohmer
I don't think so. Actually,
Still thinking about this one. It seems like a Jinja validator should be in order. /to @sebastianbenz
Correct, Prettier was used to auto-format all of the documents. Prettier uses the same parser as this linter under the hood (remark). I would just need to see where all of the defaults are defined so we can check for that. This may mean using a different preset. |
|
@sebastianbenz, in the previous meeting it was suggested that this repo should use the same mechanism as the amphtml repo when linting the Markdown. This would mean bringing the There is a slight issue with this. This gulp task depends on quite a bit of infrastructure in that repository. Rather than bringing in all of this for a single gulp task, I'd like to propose an idea that I think would be preferable: create a new package in the amp-toolbox monorepo to take care of the markdown linting for both repos. This would prevent duplication of all of this infrastructure and would guarantee that the same rules are being used for both. Does this sound like something you would like to see happen? There are a few ways this package could exist:
Another idea:
More info: I spoke to the maintainer of remark to see what his suggestion was, and he recommended using I've cc'ed Raghu since this would involve coordination between all three repos and maybe other ideas exist that I haven't thought of. /cc @rsimha |
|
TBH, I wouldn't overthink it for now. Porting the gulp task sounds like the easiest way to get started. I just remembered that I already looked into this, but never had the time to finish... Anyway, you can find my version of the prettify gulp task here: https://gist.github.com/sebastianbenz/4b4b5132893aac53831c813df0202857 |
|
Hi everyone. Glad to see that the
I agree with this statement. Trying to move common infra to a separate repo will introduce a layer of complexity to maintaining
I took a look at the list of internal dependencies of // Used only for executing a child process. You can directly use `child_process`
// from `node`.
const {exec} = require('../common/exec');
// `getFilesToCheck` is a small function you can copy. `logOnSameLine` is a helper
// util used only during local runs. You can ignore it and just use `log`.
const {getFilesToCheck, logOnSameLine} = require('../common/utils');
// This is a `node_modules` dependency.
const {green, cyan, red, yellow} = require('ansi-colors');
// This is purely for logging. You can replace `isTravisBuild` with a check for
// `process.env.TRAVIS`
const {isTravisBuild} = require('../common/travis');
// This is used to run `yarn` before tests are executed. You can ignore this for
// your Travis runs.
const {maybeUpdatePackages} = require('./update-packages');
// This can simply be `**/*.md`.
const {prettifyGlobs} = require('../test-configs/config');Hope this helps! |
|
Hey @DerekNonGeneric, can you give a quick update on this? 🙂 |
|
Sure, I am currently working on getting it to display the suggested corrections along with the errors. Almost done! Narrator: Later that day...
|
DerekNonGeneric
force-pushed
the
feat/lint-md
branch
2 times, most recently
from
75e94bf
to
245baea
Compare
|
@sebastianbenz, the 𝓹𝓻𝓮𝓽𝓽𝓲𝓯𝔂 task is ready to go! 🎉 The build will fail until someone runs |
DerekNonGeneric
force-pushed
the
feat/lint-md
branch
from
245baea
to
8f354ba
Compare
package.json
Outdated
| @@ -11,8 +11,10 @@ | |||
| "bootstrap": "npx gulp bootstrap", | |||
| "develop": "npx gulp develop", | |||
| "lint": "npx gulp lintAll", | |||
| "lint:md": "npx gulp prettify", | |||
There was a problem hiding this comment.
we should add a filter for only markdown here. We will use prettify for js etc in the future as well.
There was a problem hiding this comment.
Unclear on the API here.
Would it be gulp prettify lint --md and gulp prettify format --md?
There was a problem hiding this comment.
lint would be the default subcommand, so it could be omitted.
Alternatively, they could both be flags (--format --md).
Also, any preference between --fix and --format?
There was a problem hiding this comment.
maybe simply go with lint:prettify and don't add a file format filter
DerekNonGeneric
force-pushed
the
feat/lint-md
branch
from
8f354ba
to
5414bb5
Compare
|
Merge conflict resolved. If the file format filtering proposed is intended for this PR, I'd like to implement it prior to running the fix on all files if that's okay. May need further direction on the intended API. |
DerekNonGeneric
force-pushed
the
feat/lint-md
branch
3 times, most recently
from
bcb0257
to
b1255e6
Compare
|
@sebastianbenz, I think this one is good to go now. A couple of notable items:
WDYT? Update: I believe I have fixed it so that it will no longer ignore my exclusion glob for this directory in the future. Any autoformatting applied to these issue templates are currently remaining. There's not max line-length for Markdown files, so the effects were minimal (I checked them out). The PR should be good for merge unless you can think of anything else. |
DerekNonGeneric
force-pushed
the
feat/lint-md
branch
2 times, most recently
from
c6caf68
to
8985402
Compare
Indeed just a warning, safe to ignore. This means that the environment will not be able to make calls to the Google Custom Search Engine which will render search unusable. Credentials are correctly configured for staging and production.
That's a test case for what happens when there is an invalid search request. We've improved DX here by silencing logs during test runs.
Similar to above: we've now silenced logs during test runs. There are test cases that don't have a fully qualified frontmatter to keep the cases as narrow and simple as possible.
Safe to ignore. We cache rendered pages on staging and production by storing them in Redis instances. This behaviour is simulated by using lru-cache for all environments that don't a Redis instance configured. That's a valuable warning for prod though as it would mean the instances are not available 😉
Also a test case testing error behaviour. Safe to ignore. Additionally not finally implemented and/or productive.
Unrelated to #3477 but indeed the problem with this PR. Looking into this now. We check internal links prior builds to make sure there aren't any references that would make Grow break.
The Reference checker fails and therefore the whole build/check fails 😉 |
|
@matthiasrohmer, please ping me when you think I should circle back to this. P.S. Thanks for cleaning up the logs! |
DerekNonGeneric
force-pushed
the
feat/lint-md
branch
from
8985402
to
06f19b0
Compare
DerekNonGeneric
force-pushed
the
feat/lint-md
branch
from
06f19b0
to
9035d47
Compare
|
Woohoo! Thanks a lot! |
|
I will have to separate these commits out a little better. I had trouble importing all of the documents. Not all of them came in when running |
DerekNonGeneric
force-pushed
the
feat/lint-md
branch
from
c489ca3
to
d5ab48a
Compare
DerekNonGeneric
force-pushed
the
feat/lint-md
branch
from
d5ab48a
to
11f037a
Compare
|
@matthiasrohmer PTAL, I think this PR still needs your approval. This would still need to be integrated into one of the tasks (lint/format) and perhaps run as part of the pre-push Husky hook. I hope this can come later though. I don't want this PR to sit for much longer. |
|
|
This PR adds the capability to lint the Markdown files in this project. It would be a feature of the infrastructure used to build the site and should not cause noticeable side effects to the rendered pages.
I hope to bring this to ampproject/wg-outreach#51 for discussion.
A few items I'd like to discuss:
Is the Node.js preset acceptable?