Skip to content

test: pin phrasal-verb handling of "rollback" and "comeback" (#3239) - #3884

Merged
hippietrail merged 1 commit into
Automattic:masterfrom
lukiod:fix/rollback-phrasal-verb
Jul 26, 2026
Merged

test: pin phrasal-verb handling of "rollback" and "comeback" (#3239)#3884
hippietrail merged 1 commit into
Automattic:masterfrom
lukiod:fix/rollback-phrasal-verb

Conversation

@lukiod

@lukiod lukiod commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Re: #3239

What I found

I went to fix this and found it already works on master. Both sentences from the issue behave correctly now:

"I already did a rollback. I'm not going to rollback again."
  -> "I already did a rollback. I'm not going to roll back again."

"I already did a comeback. I'm not going to comeback again."
  -> "I already did a comeback. I'm not going to come back again."

The dictionary entries are clean rollback/~NSg and comeback/~NgS, no V flag so PhrasalVerbAsCompoundNoun reaches them and the generic particle logic does the rest (back is in particle_endings, and roll/come both carry V).

So this issue looks resolvable as fixed, and I didn't want to open a PR pretending otherwise.

What this PR adds instead

Regression tests, because nothing currently pins it. I grepped there is no test mentioning rollback or comeback anywhere in the suite.

That gap seemed worth closing specifically because of how this bug worked: a stray verb flag in the dictionary silently disables the rule for that word. It throws no error, and the linter keeps passing all its other tests, so the regression would be invisible until someone reported it again.

Three tests using the issue's own sentences:

  • flag_rollback_used_as_a_verb_issue_3239
  • flag_comeback_used_as_a_verb_issue_3239
  • dont_flag_rollback_or_comeback_as_nouns_issue_3239 negative case, so the nouns themselves stay unflagged

cargo test -p harper-core --lib: 5677 passed, 0 failed.

Happy to close this if you'd rather just close the issue without the tests, or to fold them into an existing test if you prefer them grouped differently.

…tic#3239)

Both words are compound nouns whose dictionary entries once carried a
stray verb flag inherited from an old Wiktionary import. While that flag
was present the linter treated them as legitimate verbs and skipped them,
so "I'm not going to rollback again" went unflagged.

The dictionary entries are correct now and PhrasalVerbAsCompoundNoun
handles both words, but nothing in the suite pinned that. The failure mode
is worth guarding specifically: a data flag silently disabling a rule
produces no error anywhere, and the rule keeps passing its other tests.

Uses the sentences from the issue, plus a negative case so the nouns
themselves stay unflagged.

5677 tests pass.
@hippietrail

hippietrail commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator

You're right. Some intermediate change must've fixed it by coincidence. I'm just going to quickly test locally then I'm pretty sure I should be able to merge this. Thanks.

Oh I just noticed your observation about the dictionary. That rings a bell. I know I have spotted V flags where they shouldn't be sometimes. Some on verb forms other than base forms, dating back to before Harper fully grokked how those word, but sometimes due to probably Wiktionary data imports probably into the Hunspell dictionary before it got forked into the Harper dictionary. Wiktionary sometimes has "common mistake" entries, which some dictionary parsers do not filter out. (Wiktionary is very hairy to parse.)

@hippietrail
hippietrail added this pull request to the merge queue Jul 26, 2026
Merged via the queue into Automattic:master with commit f8ff7a8 Jul 26, 2026
12 checks passed
@lukiod
lukiod deleted the fix/rollback-phrasal-verb branch July 26, 2026 15:45
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