Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add linting to CI for martin book #1030

Merged
merged 38 commits into from
Dec 4, 2023
Merged

Conversation

sharkAndshark
Copy link
Collaborator

@sharkAndshark sharkAndshark commented Nov 29, 2023

Try to fix #979 with markdownlint-cli2 and markdown-link-check

Link check

  • Add link check to just clippy
  • Add relative links check with markdown-link-check
  • Ignore CHANGELOG.md
  • Ignore localhost and 127.0.0.1
  • Ignore http://opensource.org
  • Add to justfile

Lintting

@sharkAndshark
Copy link
Collaborator Author

How about we ignore the Line Length rule? @nyurik

Copy link
Member

@nyurik nyurik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, thanks! Could you rename the config file though - it should not have a . in front of the name. The dot is only used when you want to hide the file, e.g. if its in the root of the repo. Thx!

@sharkAndshark
Copy link
Collaborator Author

sharkAndshark commented Nov 30, 2023

Some rules do we need to ignore / finetune?

rule description the doc which violate this
First line in a file should be a top-level heading This rule is intended to ensure documents have a title and is triggered when the first line in the file isn't a top-level (h1) heading: docs/src/installation.md:1
Inline HTML This rule is triggered whenever raw HTML is used in a Markdown document: docs/src/env-vars.md:11:287

I prefer to ignore the Inline HTML rule as I think the <br/> is really useful. And I'm not sure the First line rule.

@nyurik
Copy link
Member

nyurik commented Nov 30, 2023

I don't think either of these two rules are that important to us. Simpler is better. The more significant aspect would be to make sure all URLs are working. The rest is just "nice to have". Thx!

Copy link
Member

@nyurik nyurik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great, thanks! I left a few minor comments. Also, please modify justfile:

  • modify clippy target to run URL link checker and format checker. Might be OK to just verify markdown formatting.
  • add fmt-docs (or maybe modify fmt?) to reformat all *.md files

.github/files/markdown.links.config.json Outdated Show resolved Hide resolved
.github/workflows/build-deploy-docs.yml Outdated Show resolved Hide resolved
@nyurik
Copy link
Member

nyurik commented Dec 1, 2023

P.S. If the license link http://opensource.org/licenses/MIT is too much of a problem, just add it to the ignore list.

@nyurik
Copy link
Member

nyurik commented Dec 1, 2023

I just tried it with curl -vL http://opensource.org/licenses/MIT and I also see a 403. I think that site uses javascript redirect or some other weird thing

@sharkAndshark
Copy link
Collaborator Author

I'm confused. Both work on my pc.

markdown-link-check ./README.md -c /home/zhangyijun/repos/martin/.github/files/markdown.links.config.json
..
  [✓] http://www.apache.org/licenses/LICENSE-2.0
  [✓] LICENSE-MIT
  [✓] http://opensource.org/licenses/MIT
...
curl -vL http://opensource.org/licenses/MIT
...
<div id="LicenseText">
<p>Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the &#8220;Software&#8221;), to deal in the Software without restriction, including without limitation the rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons to whom the Software is furnished to do so, subject to the following conditions:</p>
<p>The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.</p>
<p>THE SOFTWARE IS PROVIDED &#8220;AS IS&#8221;, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.</p>
</div>

@nyurik
Copy link
Member

nyurik commented Dec 1, 2023

Hm, I ran it again -- you can see that it returns 403 (the -I option prints just the headers)

✦ ❯ curl -IL http://opensource.org/licenses/MIT
HTTP/1.1 301 Moved Permanently
Date: Fri, 01 Dec 2023 07:52:20 GMT
Connection: keep-alive
Cache-Control: max-age=3600
Expires: Fri, 01 Dec 2023 08:52:20 GMT
Location: https://opensource.org/licenses/MIT
Report-To: {"endpoints":[{"url":"https:\/\/a.nel.cloudflare.com\/report\/v3?s=xmkkYfend9Wqn3UJlmGug5rsx5NHMqnb5R30ZFdLgA855LqPn59shvg2lcna6TMeAWJP5ws4XMq5MMI%2B3jl%2BeabzdY59QDBjLEvlFl0WhgI8AacysPmTtm6DtEkRAqHhCjIrQ%2F91%2FXU6zQ5e4A%3D%3D"}],"group":"cf-nel","max_age":604800}
NEL: {"success_fraction":0,"report_to":"cf-nel","max_age":604800}
Server: cloudflare
CF-RAY: 82e9c00b1f503320-EWR
alt-svc: h3=":443"; ma=86400

HTTP/2 403
date: Fri, 01 Dec 2023 07:52:21 GMT
content-type: text/html; charset=UTF-8
cross-origin-embedder-policy: require-corp
cross-origin-opener-policy: same-origin
cross-origin-resource-policy: same-origin
origin-agent-cluster: ?1
permissions-policy: accelerometer=(),autoplay=(),browsing-topics=(),camera=(),clipboard-read=(),clipboard-write=(),geolocation=(),gyroscope=(),hid=(),interest-cohort=(),magnetometer=(),microphone=(),payment=(),publickey-credentials-get=(),screen-wake-lock=(),serial=(),sync-xhr=(),usb=()
referrer-policy: same-origin
x-frame-options: SAMEORIGIN
cf-mitigated: challenge
cache-control: private, max-age=0, no-store, no-cache, must-revalidate, post-check=0, pre-check=0
expires: Thu, 01 Jan 1970 00:00:01 GMT
report-to: {"endpoints":[{"url":"https:\/\/a.nel.cloudflare.com\/report\/v3?s=g8S%2FiFtd9uAC1Qtlc3HBTobyL%2BIcJ6PlDcpbAtBqayV8e4SxQgveEE7H%2BDGlor%2BZaydTu%2B6Jib43m0KFcjBRVwhwXZ1AhmYZx9i3s3JccqfJy8pg1f%2FH0so2T3zYVkMHz1UGJG%2FJAEUkSzhPfA%3D%3D"}],"group":"cf-nel","max_age":604800}
nel: {"success_fraction":0,"report_to":"cf-nel","max_age":604800}
server: cloudflare
cf-ray: 82e9c00ccfa75e66-EWR
alt-svc: h3=":443"; ma=86400

@nyurik
Copy link
Member

nyurik commented Dec 1, 2023

P.S. I suspect this is due to some CloudFlare anti-DDOS process that uses javascript to redirect to the right page

@sharkAndshark
Copy link
Collaborator Author

sharkAndshark commented Dec 2, 2023

P.S. I suspect this is due to some CloudFlare anti-DDOS process that uses javascript to redirect to the right page

403 still after many hours. Ignored now.

@sharkAndshark sharkAndshark marked this pull request as ready for review December 2, 2023 05:35
Copy link
Member

@nyurik nyurik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I absolutely love the clean markdown, thanks!

I don't think NPM requirement for development is worth it just for markdown, especially considering how flaky NPM can be. Instead, lets use docker containers for this. Docker with docker-compose is already a dependency. We can do it two different ways:

  • from justfile, use docker run --rm -it ... command for formatting and linting
  • from justfile, use docker-compose run and add two new sections to the docker-compose.

I think the first path might be easier.

See https://github.com/DavidAnson/markdownlint-cli2#container-image for info on at least one of the tools.

.github/files/markdown.links.config.json Outdated Show resolved Hide resolved
justfile Outdated Show resolved Hide resolved
justfile Outdated Show resolved Hide resolved
justfile Outdated Show resolved Hide resolved
@@ -0,0 +1,18 @@
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for consistency, lets rename this file as .github/files/markdownlint-cli2.jsonc

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect we couldn't do this:

@sharkAndshark ➜ /workspaces/martin (doc_style_check) $ markdownlint-cli2 --config ./.github/files/markdownlint-cli2.jsonc --fix
markdownlint-cli2 v0.11.0 (markdownlint v0.32.1)
Error: Configuration file "/workspaces/martin/.github/files/markdownlint-cli2.jsonc" is unrecognized; its name should be (or end with) one of the supported types (e.g., ".markdownlint.json" or "example.markdownlint-cli2.jsonc").
    at readOptionsOrConfig (/usr/local/share/nvm/versions/node/v20.8.1/lib/node_modules/markdownlint-cli2/markdownlint-cli2.js:191:11)
    at main (/usr/local/share/nvm/versions/node/v20.8.1/lib/node_modules/markdownlint-cli2/markdownlint-cli2.js:935:13)
    at /usr/local/share/nvm/versions/node/v20.8.1/lib/node_modules/markdownlint-cli2/markdownlint-cli2.js:1045:32
    at run (/usr/local/share/nvm/versions/node/v20.8.1/lib/node_modules/markdownlint-cli2/markdownlint-cli2.js:1050:5)
    at Object.<anonymous> (/usr/local/share/nvm/versions/node/v20.8.1/lib/node_modules/markdownlint-cli2/markdownlint-cli2.js:1062:3)
    at Module._compile (node:internal/modules/cjs/loader:1241:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1295:10)
    at Module.load (node:internal/modules/cjs/loader:1091:32)
    at Module._load (node:internal/modules/cjs/loader:938:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:83:12)
// Read an options or config file in any format and return the object
const readOptionsOrConfig = async (configPath, fs, noRequire) => {
  const basename = pathPosix.basename(configPath);
  const dirname = pathPosix.dirname(configPath);
  let options = null;
  let config = null;
  if (basename.endsWith(".markdownlint-cli2.jsonc")) {
    const jsoncParse = await getJsoncParse();
    options = jsoncParse(await fs.promises.readFile(configPath, utf8));
  } else if (basename.endsWith(".markdownlint-cli2.yaml")) {
    options = yamlParse(await fs.promises.readFile(configPath, utf8));
  } else if (
    basename.endsWith(".markdownlint-cli2.cjs") ||
    basename.endsWith(".markdownlint-cli2.mjs")
  ) {
    options = await (
      importOrRequireConfig(fs, dirname, basename, noRequire, noop)()
    );
  } else if (
    basename.endsWith(".markdownlint.jsonc") ||
    basename.endsWith(".markdownlint.json") ||
    basename.endsWith(".markdownlint.yaml") ||
    basename.endsWith(".markdownlint.yml")
  ) {
    const jsoncParse = await getJsoncParse();
    config =
      await markdownlintReadConfig(configPath, [ jsoncParse, yamlParse ], fs);
  } else if (
    basename.endsWith(".markdownlint.cjs") ||
    basename.endsWith(".markdownlint.mjs")
  ) {
    config = await (
      importOrRequireConfig(fs, dirname, basename, noRequire, noop)()
    );
  } else {
    throw new Error(
      `Configuration file "${configPath}" is unrecognized; ` +
      "its name should be (or end with) one of the supported types " +
      "(e.g., \".markdownlint.json\" or \"example.markdownlint-cli2.jsonc\")."
    );
  }

  if (options) {
    if (options.config) {
      options.config = await getExtendedConfig(options.config, configPath, fs);
    }
    return options;
  }

  config = await getExtendedConfig(config, configPath, fs);
  return { config };
};

Copy link
Member

@nyurik nyurik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome job, thanks! I made a minor adjustment in the justfile to make lints run faster with parallel execution and a single docker run command (they are slow to start up)

@nyurik nyurik enabled auto-merge (squash) December 4, 2023 22:01
@nyurik nyurik merged commit 4814d33 into maplibre:main Dec 4, 2023
18 checks passed
@sharkAndshark sharkAndshark deleted the doc_style_check branch March 14, 2024 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce style checking and linting for martin book
2 participants