bitcoin icon indicating copy to clipboard operation
bitcoin copied to clipboard

ci: remove `doc/release-notes.md` from lint-spelling.py

Open JeremyRubin opened this issue 1 month ago • 11 comments

/doc/release-notes/ is ignored, but during release branch-off, the latest release is copied to doc/release-notes.md. This means that CI runs based off of the branch off point (e.g., for backport dev) will fail CI if there are spelling errors, or if there are e.g. contributors names that resemble spelling errors (not a hypothetical example).

JeremyRubin avatar Nov 29 '25 14:11 JeremyRubin

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33968.

Reviews

See the guideline for information on the review process.

Type Reviewers
User requested bot ignore maflcko

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #34053 (lint: Remove confusing, redundant, and brittle lint-spelling by maflcko)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot avatar Nov 29 '25 14:11 DrahtBot

The release notes is kinda' important to check for spelling errors - could we maybe add the exceptional contributor names to https://github.com/bitcoin/bitcoin/blob/master/test/lint/spelling.ignore-words.txt instead?

l0rinc avatar Nov 30 '25 10:11 l0rinc

I think it's a separate issue -- since it seems that the release (e.g., non code doc changes pre-release) is maybe not getting run through CI anyways, or else this would have been caught?

JeremyRubin avatar Nov 30 '25 15:11 JeremyRubin

(not a hypothetical example).

I tried searching for a real example in the past, but I couldn't find one: https://github.com/bitcoin/bitcoin/pulls?q=%22LLM+reason+%28%E2%9C%A8+experimental%29%22+sort%3Aupdated-desc+is%3Apr+draft%3Afalse+label%3ABackport+is%3Aclosed+

Not sure how much value there is in codespell at this point, given the LLM typo linter, so codespell could even be removed fully. It also seems fine to keep as-is and deal with true and false positives in the backport pull request, when there is need.

Also, seems fine to exclude it, like here. No strong opinion, but lgtm ACK 6494713d2d48926066972376df44a0b6166a223a

<!--meta-tag:bot-skip-->

maflcko avatar Dec 01 '25 08:12 maflcko

so codespell could even be removed fully

It does seem to cause more problems than it's solving, I'd vote for that instead

l0rinc avatar Dec 01 '25 08:12 l0rinc

will fail CI if there are spelling errors

I don't think this is true either. The comment in the file literally says:

Note: Will exit successfully regardless of spelling errors.

maflcko avatar Dec 01 '25 08:12 maflcko

I guess it's not a failure, as it only pops up when other issues flag, but it does get reported in CI as:

^---- ⚠️ Failure generated from lint-tests.py
doc/release-notes.md:379: Atack ==> Attack

which is sort of confusing

JeremyRubin avatar Dec 03 '25 15:12 JeremyRubin

That could be fixed by rewording, or changing the spacing, maybe something like:

diff --git a/test/lint/test_runner/src/main.rs b/test/lint/test_runner/src/main.rs
index 5c65fcad89..39ae9dc815 100644
--- a/test/lint/test_runner/src/main.rs
+++ b/test/lint/test_runner/src/main.rs
@@ -742,7 +742,7 @@ fn run_all_python_linters() -> LintResult {
                 .success()
         {
             good = false;
-            println!("^---- ⚠️ Failure generated from {entry_fn}");
+            println!("^---- ⚠️ Failure generated from {entry_fn}\n\n");
         }
     }
     if good {

maflcko avatar Dec 03 '25 15:12 maflcko

are you still working on this?

maflcko avatar Dec 09 '25 11:12 maflcko

no, because it's mergeable as is, which is excluding the lint rule to release-notes.md as it is excluded to draft release notes, which is fine.

JeremyRubin avatar Dec 09 '25 18:12 JeremyRubin

no, it is not mergeable, because it is wrong. The CI does not fail when there are spelling errors, but this was mentioned above.

excluding the lint rule to release-notes.md as it is excluded to draft release notes,

Draft release notes are not excluded either.

Downgrading my review to a NACK for now. I think it could help if you supported your claims with exact steps to reproduce, so that the behavior is clear, and the bug that you are trying to fix is also clear and reproducible.

maflcko avatar Dec 09 '25 19:12 maflcko