retextjs / retext-spell

plugin to check spelling
https://unifiedjs.com
MIT License
73 stars 16 forks source link

Add support for ignoring times (`12:34`) `ignoreDigits` is on #25

Closed danielgolden closed 2 years ago

danielgolden commented 2 years ago

Initial checklist

Description of changes


Closes #24 and a step toward resolving newrelic/one-core-toolbox#38

github-actions[bot] commented 2 years ago

Hi! It seems some of the things asked in the template are missing? Please edit your post to fill out everything.

You won’t get any more notifications from me, but I’ll keep on updating this comment, and remove it when done!

If you need it, here’s the original template ```markdown ### Initial checklist * [ ] I read the support docs * [ ] I read the contributing guide * [ ] I agree to follow the code of conduct * [ ] I searched issues and couldn’t find anything (or linked relevant results below) * [ ] If applicable, I’ve added docs and tests ### Description of changes TODO ```

Thanks, — bb

codecov-commenter commented 2 years ago

Codecov Report

Merging #25 (5003a00) into main (0b85545) will not change coverage. The diff coverage is 100.00%.

:exclamation: Current head 5003a00 differs from pull request most recent head 80c0488. Consider uploading reports for the commit 80c0488 to get more accurate results

@@            Coverage Diff            @@
##              main       #25   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines          291       304   +13     
=========================================
+ Hits           291       304   +13     
Impacted Files Coverage Δ
index.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0b85545...80c0488. Read the comment docs.

danielgolden commented 2 years ago

That's true, and a good point. There aren't really other words I want to ignore (times are my priority), they just seemed to fit in the same category as I was thinking about it (but I supposed this is exactly how scope creep happens haha).

I was thinking more of adding /^\d{1,2}:\d{2}(?:[ap].?m)?$/i. Either under the existing ignoreDigits option

I really like this solution. Keeps the API simple and it fit's well the specific problem of ignoring times. I'll update the PR to reflect these changes.

Thanks for the feedback! :)

danielgolden commented 2 years ago

I'm not confident about the documentation updates. Is this a satisfactory way to document the ignoring of times? If so, awesome. If not, would love your opinion.

https://github.com/retextjs/retext-spell/blob/d71c22bf2917b28ade1157eb2c530e9a35002614/index.js#L18-L19 https://github.com/retextjs/retext-spell/blob/d71c22bf2917b28ade1157eb2c530e9a35002614/readme.md?plain=1#L112-L113

github-actions[bot] commented 2 years ago

Hi! This was closed. Team: If this was merged, please describe when this is likely to be released. Otherwise, please add one of the no/* labels.

wooorm commented 2 years ago

Thanks, released in 5.3.0!

danielgolden commented 2 years ago

Awesome! Thanks so much :)