search

drahnr / cargo-spellcheck

Checks all your documentation for spelling and grammar mistakes with hunspell and a nlprule based checker for grammar
Apache License 2.0
320 stars 33 forks source link

Implement Optional Footnote References Ignore #311

Closed petervaro closed 11 months ago

petervaro commented 11 months ago

What does this PR accomplish?

Changes proposed by this PR:

This PR introduces the option in the configuration to skip checking the footnote references, i.e. under [Hunspell.quirks] it introduces a new option, called check_footnote_references which is by default set to true for backwards compatibility and can be explicitly opted out by setting it to false.

As the doc-comment explains it, this is very useful when the markdown uses abbreviations as footnote references (akin to how one would use short link references) but unlike link references this is never skipped. Which results in the word the footnote had been applied to concatenated with the footnote reference itself, e.g.

Hello[^abc].

Will result in checking for spelling against the segment Helloabc.

📜 Checklist

drahnr commented 11 months ago

Hey @petervaro , thank you for your contribution! I did a quick check and it seems the tokenization doesn't quite work as anticipated in this case https://github.com/drahnr/cargo-spellcheck/pull/312

The testsuite is not known to be flaky.

Could you possibly reference the commonmark spec where the syntax is outlined? I am not an avid footnote user.

petervaro commented 11 months ago

[...] I did a quick check and it seems the tokenization doesn't quite work as anticipated in this case #312

I'm not exactly sure what exactly is missing from the tokenisation, but my tests that I added for the footnotes and the additional configuration I implemented are working as expected. :thinking:

The testsuite is not known to be flaky.

I believe I made a mistake there by hastily replacing the explicit Default implementation for hunspell::Quirks to a derived one which assigns falses for both the allow_emojis and the check_footnote_references, whereas the original implementation assigns true for the former, and indeed the latter, introduced by me here, should also be that.

Could you possibly reference the commonmark spec where the syntax is outlined? I am not an avid footnote user.

I'm afraid I cannot. The CommonMark spec does not contain any details on footnotes as it is considered a so called extension, however the specification fails to specify what the extensions are. That being said, we explicitly enable footnotes in doc-chunks/src/markdown.rs:158 and we respond to the FootnoteReference event in doc-chunks/src/markdown.rs:327.

Even though GHFM, GLFM, and even rustdoc support them, just to name a few. Based on these, the specification could be something like the following:

A footnote reference has the same specification as the link reference, except that the link label must start with the character ^.

Finally all checks are passing for my PR, however I'm still not completely satisfied with what I have here. The contribution guide (or the lack thereof) gives me no clue as to what to prefer: remain backwards compatible at all cost (this is what I did in this PR), or fix mistakes even if that alters the previous behaviour.

Why does this matter? The former means the merge of the word and footnote reference has to be disabled explicitly, but the configuration option check_footnote_references implies that the references themselves are going to be checked, which is not the case, like I said, the mistakenly merged word is going to be. The latter would mean we would skip checking the footnote references altogether, the same way as we already do with the links.

IMHO the latter feels like a better option to me, but I wanted to stay on the safe side first. Let me know your preference and if you decide that the former is the way to go, I'm open for suggestions as to how to call this option in the configuration.

drahnr commented 11 months ago

Hey, it depends on a case by case basis, I am also happy to just do a breaking change release. I agree that this is a common extension and should be supported without any headaches.

Thank you for your contribution!

I'll add a contributing guide some time next week~ish.