Repository navigation
Add Markdown link checking and a PR-template doc-update reminder - #738
KumarShivam1908 wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Thanks @KumarShivam1908, and thanks @beanscg for #655! Both asks from my review there are in and I ran it locally to confirm.
I have some inline comments below, and one more general question:
@observingClouds could you weigh in on one thing: The hook fetches its binary by piping cargo-bins/cargo-binstall@main into bash, from an unpinned branch, and that runs during linting both locally and in CI. It is cached after the first run and rev pins the lychee version itself, so this is not obviously a problem, but it is a new third party in the chain. Do you have any security concerns about that, or is it acceptable for a pre-commit hook to fetch its own binary this way?
Two more asks below, which I could not leave as inline suggestions because neither file is in the diff.
.github/workflows/pre-commit.yml
The hook lands in a Python 3.10-3.14 matrix and lychee has no Python dependency, so as it stands we would repeat the same ~78 live requests five times per push, and fetch the binary into five separate caches. SKIP takes it out of the matrix and one extra job runs it once:
jobs:
pre-commit-job:
runs-on: ubuntu-latest
+ env:
+ SKIP: lychee
strategy:
matrix:
python-version: ["3.10", "3.11", "3.12", "3.13", "3.14"]
steps:
- uses: actions/checkout@v2
- name: Set up Python
uses: actions/setup-python@v2
with:
python-version: ${{ matrix.python-version }}
- uses: pre-commit/action@v3.0.1
+
+ link-check-job:
+ runs-on: ubuntu-latest
+ steps:
+ - uses: actions/checkout@v2
+ - name: Set up Python
+ uses: actions/setup-python@v2
+ with:
+ python-version: "3.13"
+ - uses: pre-commit/action@v3.0.1
+ with:
+ extra_args: lychee --all-filesREADME.md line 604
We have two different Slack invites: the badge on line 1 and both CONTRIBUTING.md references use kutt.to/mllam, but this line hardcodes join.slack.com/...zt-2t112zvm8-.... The shortener currently points at zt-3tkm6wn34-..., so they are already out of step, and per the exclude comment nothing will ever tell us if one dies. Could you point this one at the shortener too, so the invite token lives in kutt.to and rotating it never needs a repo change?
-There is an open [mllam slack channel](https://join.slack.com/t/ml-lam/shared_invite/zt-2t112zvm8-Vt6aBvhX7nYa6Kbj_LkCBQ) that anyone can join
+There is an open [mllam slack channel](https://kutt.to/mllam) that anyone can join| args: ["--no-progress", "--config", ".github/lychee.toml"] | ||
| types: [markdown] |
There was a problem hiding this comment.
pass_filenames is needed: touch no markdown, hook does not run. Touch one markdown file, everything gets checked.
--hidden is needed for .github/pull_request_template.md.
| args: ["--no-progress", "--config", ".github/lychee.toml"] | |
| types: [markdown] | |
| args: ["--no-progress", "--config", ".github/lychee.toml", "--hidden", "."] | |
| types: [markdown] | |
| pass_filenames: false |
There was a problem hiding this comment.
Applied. Confirmed --hidden matters — 238 links found with it, 237 without, the difference being the PR template.
| - [ ] For any new/modified functions/classes I have added docstrings that clearly describe its purpose, expected inputs and returned values | ||
| - [ ] I have placed in-line comments to clarify the intent of any hard-to-understand passages of my code | ||
| - [ ] I have updated the [README](README.MD) to cover introduced code changes | ||
| - [ ] I have updated the [README](../README.md) to cover introduced code changes |
There was a problem hiding this comment.
GitHub inserts the template into the PR body verbatim and does not rewrite relative links, so from a PR page it resolves to github.com/mllam/neural-lam/README.md, which 404ed for me. I suggest to drop the link, matching the item you add on the next line:
| - [ ] I have updated the [README](../README.md) to cover introduced code changes | |
| - [ ] I have updated the README to cover introduced code changes |
There was a problem hiding this comment.
Applied. My ../README.md fixed the case bug but broke in the rendered body, as you found , plain text avoids both.
| # Reachable for humans, but reject automated requests (bot filtering, | ||
| # link shorteners), so they would fail the check even while working. | ||
| '^https://join\.slack\.com/t/ml-lam/shared_invite/', | ||
| '^https://kutt\.to/mllam$', | ||
| '^https://pyg\.org/?$', |
There was a problem hiding this comment.
Slack answers 403 for the real invite and for one I invented, and kutt.to answers 200 for short links that were never created. So no HTTP check can tell live from dead here.
| # Reachable for humans, but reject automated requests (bot filtering, | |
| # link shorteners), so they would fail the check even while working. | |
| '^https://join\.slack\.com/t/ml-lam/shared_invite/', | |
| '^https://kutt\.to/mllam$', | |
| '^https://pyg\.org/?$', | |
| # Unverifiable over HTTP, so checking them would only add flakiness: | |
| # join.slack.com answers 403 for live and dead invites alike, and | |
| # kutt.to answers 200 for short links that do not exist. | |
| '^https://join\.slack\.com/t/ml-lam/shared_invite/', | |
| '^https://kutt\.to/mllam$', | |
| # Times out on automated requests, both the apex domain and www. | |
| '^https://(www\.)?pyg\.org/?$', |
| # CHANGELOG's auto-generated self-references: ~142 links that always | ||
| # resolve, and dominate the runtime if re-checked on every commit. | ||
| # `NNN` is the placeholder in CONTRIBUTING.md's changelog example. | ||
| '^https://github\.com/mllam/neural-lam/(issues|pull)/(\d+|NNN)/?$', |
There was a problem hiding this comment.
I could not reproduce the NNN failure. I ran the tree with \d+ and with (\d+|NNN) and got identical results. I suggest to revert to what I proposed in #655:
| # CHANGELOG's auto-generated self-references: ~142 links that always | |
| # resolve, and dominate the runtime if re-checked on every commit. | |
| # `NNN` is the placeholder in CONTRIBUTING.md's changelog example. | |
| '^https://github\.com/mllam/neural-lam/(issues|pull)/(\d+|NNN)/?$', | |
| # CHANGELOG's auto-generated self-references: ~147 links that always | |
| # resolve, and dominate the runtime if re-checked on every commit. | |
| '^https://github\.com/mllam/neural-lam/(issues|pull)/\d+/?$', |
How did you get the error, I might be in the wrong here...?
There was a problem hiding this comment.
You're not in the wrong, I am , I never saw that failure.
I curl'd pull/NNN and got a 404, so I assumed lychee would flag it and added the alternation without ever running it. But that link sits inside a fenced ```markdown block in CONTRIBUTING.md, so lychee never requests it. I checked whether the URL would fail, not whether lychee would ask.
Re-ran both ways and got your result: identical, 238 Total, 0 Errors. Reverted to your version and dropped the claim from the PR description
|
|
||
| # Also verify `#section` anchors resolve, not just that the file exists. | ||
| include_fragments = "full" | ||
| timeout = 20 |
There was a problem hiding this comment.
timeout = 20 is exactly lychee's default, so it is a no-op. I propose to spend the line on caching instead, which took a repeat run here from 2.5s to 16ms. Then also add .lycheecache to .gitignore.
| timeout = 20 | |
| cache = true | |
| max_cache_age = "2d" |
There was a problem hiding this comment.
Applied. Confirmed that 20 is lychee’s default, so that line wasn’t actually doing anything. With caching, it went from ~4.8s cold to ~0.9s warm, and .lycheecache is gitignored.
|
|
||
| ### Maintenance | ||
|
|
||
| - Add a `lychee` pre-commit hook that checks Markdown links and section anchors (configured in `.github/lychee.toml`), add a PR-template checklist item reminding contributors to update `CONTRIBUTING.md` / `AGENTS.md` when they change contributor workflow, and fix the two broken links the new hook surfaced [\#738](https://github.com/mllam/neural-lam/pull/738) @KumarShivam1908 |
There was a problem hiding this comment.
I suggest to keep it to the two things a reader cares about:
| - Add a `lychee` pre-commit hook that checks Markdown links and section anchors (configured in `.github/lychee.toml`), add a PR-template checklist item reminding contributors to update `CONTRIBUTING.md` / `AGENTS.md` when they change contributor workflow, and fix the two broken links the new hook surfaced [\#738](https://github.com/mllam/neural-lam/pull/738) @KumarShivam1908 | |
| - Add a `lychee` pre-commit hook checking Markdown links and section anchors (config in `.github/lychee.toml`, run once per push in CI) and a PR-template reminder to update `CONTRIBUTING.md` / `AGENTS.md` [\#738](https://github.com/mllam/neural-lam/pull/738) @KumarShivam1908 |
Worth updating the PR description too, which still leans on "the two broken links the hook surfaced" as evidence.
There was a problem hiding this comment.
Applied, and updated the PR description too.
|
the next step would then be to propose this PR for a future milestone which we then discuss in the next dev meeting. FYI |
Both of the non-inline asks are in:
I also updated the PR description since it was relying on the NNN claim and “two broken links” as evidence. One note: CI still hasn’t run — the workflows are stuck on |
sadamov
left a comment
sadamov
left a comment
There was a problem hiding this comment.
great! thanks for the careful revision, this is now marked as ready and as soon as v0.7.0 releases we can merge this one into main. Usually the community accepts ready PRs that are proposed for the next milestone at the next dev-meeting.
3 participants
Describe your changes
Implements the two mechanisms requested in #654.
1. Markdown link checker in pre-commit
Adds the
lycheehook, configured via.github/lychee.toml. It validates the three things the issue lists: cross-file references, section anchors (include_fragments = "full", so a renamed heading is caught rather than silently orphaningAGENTS.md->CONTRIBUTING.md#before-you-push), and external URLs.The hook runs with
pass_filenames: falseand--hidden ., so any Markdown edit re-checks the whole tree. That is the case worth catching: renaming a heading breaks a link in a file the commit never touched, which per-file checking misses.--hiddenis required or.github/is skipped on traversal.cache = truewithmax_cache_age = "2d"keeps the repeat cost at ~0.2s.The config excludes two sets of links:
join.slack.comanswers the same for live and dead invites, andkutt.toanswers the same for short links that were never created.pyg.orgtimes out on automated requests. These are excluded rather than removed, so they keep working in the docs.mllam/neural-lam/(issues|pull)/Nself-references, which are ~150 of CHANGELOG's 162 links, auto-generated and always resolving.In CI the hook is
SKIPped in the Python 3.10-3.14 lint matrix and runs once in a separatelink-check-job. lychee has no Python dependency, so running it per matrix leg would repeat the same ~78 live requests five times per push and populate five separate binary caches.2. PR template reminder
Adds the checklist item from the issue, verbatim:
3. Link fixes surfaced along the way
README.mdlinkedtests/datastore_examples/mdp/danra.datastore.yaml; the file is atmdp/danra_100m_winds/danra.datastore.yaml. The hook does not pass onmainwithout this.[README](README.MD)link is now plain text. The original 404s on case-sensitive checkouts, and a relative path cannot work either, because GitHub inlines this template into the PR body verbatim without rewriting links.README.mdline 604 hardcoded a Slack invite token while the badge and bothCONTRIBUTING.mdreferences usekutt.to/mllam. The two were already out of sync (kutt.tonow points atzt-3tkm6wn34-..., line 604 hadzt-2t112zvm8-...), and since the invite is excluded from checking, nothing would have reported it. All Slack references now go through the shortener.Dependencies: none.
lycheeis fetched by pre-commit at the pinnedlychee-v0.24.2tag; no runtime ordevdependency changes.Relation to #655
This continues @beanscg's PR #655, which stalled with review feedback outstanding. Both of @sadamov's asks there are applied: the config lives at
.github/lychee.tomlwith a matching--configpath, and the CHANGELOG self-reference exclude is in.Verification
pre-commit run --all-files: 17 hooks, 0 failures238 Total, 212 Unique, 78 OK, 0 Errors, 160 Excluded- confirms the hook is checking, not passing vacuouslyAGENTS.md: both reported with file and line--hiddenconfirmed necessary: 238 links found with it, 237 without, the difference being.github/pull_request_template.mdSKIP=lychee pre-commit run --all-filesconfirmed to skip only that hookpytest tests/test_imports.py tests/test_config.py tests/test_cli.py tests/test_time_slicing.py: 20 passedIssue Link
closes #654
Type of change
Checklist before requesting a review
pullwith--rebaseoption if possible).Checklist for reviewers
Each PR comes with its own improvements and flaws. The reviewer should check the following:
Author checklist after completed review
reflecting type of change (add section where missing):
Checklist for assignee