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

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

Merged
merged 7 commits into from
Jul 30, 2022
Merged

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

merged 7 commits into from
Jul 30, 2022

Conversation

danielgolden
Copy link
Contributor

@danielgolden danielgolden commented Jul 26, 2022

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

  • Adds a check in the ignoreDigits option which will ignore any word that is a time (e.g. 2:41pm)
  • Adds a new test to ensure that times are ignored.
  • Updates README to include the updated documentation for the ignoreDigits option.

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

@github-actions github-actions bot added the 👋 phase/new Post is being triaged automatically label Jul 26, 2022
@github-actions

This comment has been minimized.

@codecov-commenter
Copy link

codecov-commenter commented Jul 26, 2022

Codecov Report

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

❗ 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.

@github-actions github-actions bot added 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Jul 26, 2022
@danielgolden danielgolden marked this pull request as ready for review July 26, 2022 11:40
Copy link
Member

@wooorm wooorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! ✨

What you implemented is a bit different from what the original issue was (“times in common formats (e.g. 2:41PM)”).
There is a difference between:

  • finite words, which also include numbers, and can accidentally include numbers (w0rd, because the o is next to 0), which have a great solution already: the personal dictionary.
  • numerical-like, such as times, which don’t have a good solution.

Are there other words you want to ignore? Did you consider the personal dictionary (or ignore if you have to)?

I was thinking more of adding /^\d{1,2}:\d{2}(?:[ap]\.?m)?$/i. Either under the existing ignoreDigits option. Or, if you like it, perhaps a new ignoreTimes option?

We could also add the resultions you show in a test case?

@danielgolden
Copy link
Contributor Author

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! :)

- Removes the recently added prop  and stuffs
it's time ignoring functionality into the  prop.
- Updates tests to reflect the above changes
- Updates README to reflect the above changes
@danielgolden danielgolden requested a review from wooorm July 29, 2022 13:55
@danielgolden
Copy link
Contributor Author

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.

retext-spell/index.js

Lines 18 to 19 in d71c22b

* @property {boolean} [ignoreDigits=true]
* Whether to ignore “words” that contain only digits or are times, such as `123456` or `2:41pm`.

retext-spell/readme.md

Lines 112 to 113 in d71c22b

Whether to ignore “words” that contain only digits or times, such as
`123456` or `2:41pm` (`boolean?`, default `true`).

@danielgolden danielgolden changed the title Add ignore words with digits option Ignore times when ignoreDigits option is true Jul 29, 2022
Copy link
Member

@wooorm wooorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs look good!

index.js Outdated Show resolved Hide resolved
test.js Outdated Show resolved Hide resolved
test.js Outdated Show resolved Hide resolved
test.js Show resolved Hide resolved
@wooorm wooorm changed the title Ignore times when ignoreDigits option is true Add support for ignoring times (12:34) ignoreDigits is on Jul 30, 2022
@wooorm wooorm merged commit 011eab5 into retextjs:main Jul 30, 2022
@wooorm wooorm added 🦋 type/enhancement This is great to have 💪 phase/solved Post is done labels Jul 30, 2022
@github-actions

This comment has been minimized.

@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Jul 30, 2022
@wooorm
Copy link
Member

wooorm commented Jul 30, 2022

Thanks, released in 5.3.0!

@danielgolden danielgolden deleted the add-ignore-words-with-digits-option branch July 30, 2022 09:58
@danielgolden
Copy link
Contributor Author

Awesome! Thanks so much :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💪 phase/solved Post is done 🦋 type/enhancement This is great to have
Development

Successfully merging this pull request may close these issues.

Ignore times in common formats (e.g. 2:41PM)
3 participants