Skip to content

Conversation

@hippietrail
Copy link
Collaborator

Issues

#1753

Description

I started working on #1753 and then realized it was already addressed by ProgressiveNeedsBe.
But my version could handle Ive/weve/youve/theyve with the apostrophes missing.
While adding those to the existing linter I identified a few opportunities to improve the code:

  • I simplified checking for the progressive verb form by adding a missing method to SequenceExpr.
  • I got rid of an allocation used to normalize a word to lowercase before doing string comparisons.
  • I identified a situation in which the linter doesn't work and added ignored unit tests.

I also brought over all my unit tests.
I modified one of the suggestion assertions for clarify to align the expected and actual correction so we can see what's going wrong while working on linters.

How Has This Been Tested?

Unit tests.

Checklist

  • I have performed a self-review of my own code
  • I have added tests to cover my changes

Copy link
Collaborator

@elijah-potter elijah-potter left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for getting this through. I think we can safely close #1753. Feel free to reopen if you think otherwise.

panic!(
"Expected \"{transformed_str}\" to be \"{expected_result}\" after applying the computed suggestions."
);
panic!("Expected \"{expected_result}\"\n But got \"{transformed_str}\"");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, this is easier to read.

ProgressiveNeedsBe::default(),
&[
"I've been playing with languages.toml",
"I'm playing with languages.toml",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good tests!

@elijah-potter elijah-potter added this pull request to the merge queue Nov 24, 2025
Merged via the queue into Automattic:master with commit f111abe Nov 24, 2025
11 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