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

Implement Optional Footnote References Ignore #311

Merged
merged 1 commit into from
Nov 7, 2023

Conversation

petervaro
Copy link
Contributor

@petervaro petervaro commented Oct 17, 2023

What does this PR accomplish?

  • 🦚 Feature
  • 📙 Documentation

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

  • Works on the ./demo sub directory
  • Test coverage is excellent and passes
  • Documentation is thorough

@drahnr drahnr mentioned this pull request Oct 17, 2023
3 tasks
@drahnr
Copy link
Owner

drahnr commented Oct 17, 2023

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 #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
Copy link
Contributor Author

[...] 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. 🤔

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
Copy link
Owner

drahnr commented Nov 7, 2023

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.

@drahnr drahnr merged commit 351f12e into drahnr:master Nov 7, 2023
8 checks passed
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.

2 participants