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

feat: "news" used as plural #691

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

hippietrail
Copy link
Contributor

Addresses #407

Because of many lower/upper case variants this and similar might be better as a dedicated lint - or matcher.rs could be changed to handle case variants

Addresses Automattic#407

Because of many lower/upper case variants this and similar might be better as a dedicated lint - or `matcher.rs` could be changed to handle case variants
@hippietrail hippietrail marked this pull request as ready for review February 16, 2025 16:40
@hippietrail hippietrail marked this pull request as draft February 17, 2025 02:56
@hippietrail
Copy link
Contributor Author

Turning into a draft so this doesn't get merged before I fix it. I didn't think of the false positive "Back in the days of ..." but too busy to switch branches.

@hippietrail hippietrail marked this pull request as ready for review February 17, 2025 03:57
@elijah-potter
Copy link
Collaborator

elijah-potter commented Feb 18, 2025

I love the additions. Would you mind making them "real" rules in phrase_corrections?

For example:

create_linter_map_phrase!(
    News,
    ExactPhrase::from_phrase("many news are"), // Source pattern is mapped to the 
    "a lot of news is", // target string
    "MESSAGE",
    "DESCRIPTION"
);

If you'd like to go a bit further, you can optionally use the merge_linters macro to then merge them together.

@hippietrail
Copy link
Contributor Author

hippietrail commented Feb 19, 2025

I love the additions. Would you mind making them "real" rules in phrase_corrections?

For example:

create_linter_map_phrase!(
    News,
    ExactPhrase::from_phrase("many news are"), // Source pattern is mapped to the 
    "a lot of news is", // target string
    "MESSAGE",
    "DESCRIPTION"
);

If you'd like to go a bit further, you can optionally use the merge_linters macro to then merge them together.

On a tangent, why does this code ...

create_linter_map_phrase!(LoAndBehold, ExactPhrase::from_phrase("long and behold"), "lo and behold", "Did you mean `lo and behold`?", "Detects the exact phrase `long and behold` and suggests replacing it with the idiomatically correct `lo and behold`");

...n ot trigger the precommit error for having the error in its description? I just had to change the description of the modal-of lint to not have should of in the description but here long and behold in the description does not cause a problem 🫤

@hippietrail
Copy link
Contributor Author

hippietrail commented Feb 19, 2025

I love the additions. Would you mind making them "real" rules in phrase_corrections?

I just put in a bunch of time on the plural news one and it turned out to be much more combinatorial and contextual than I expected. I think a dedicated linter will be needed for it:

how many news are shown → ✅how much / ✅how many
So many news are going on these days every time we turn on our TVs → ✅So much / ❌So many pieces of

@hippietrail hippietrail marked this pull request as draft February 19, 2025 13:32
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