Automattic / Automattic/harper

`replace_with_match_case` causes problems due to working by single character index at a time

Open
#3,741 0 comments 0 reactions 0 assignees View on GitHub
bug harper-core linting
Dominant language
Rust
Stars
15.4k
Forks
627
Avg merge
1d 15h
Merged PRs (30d)
106

Description

This has come up again and again and I was pretty sure there was already a bug report about it. Maybe there is but it's named for the symptoms rather than the cause?

The cause is that there is only one method for generating a `Suggestion` that should "match" the case of the flagged error, and that method matches case on a character-by-character basis.

This works fine most of the time when the word is all-lowercase, all caps, or when it's just one word with its first letter uppercase and the rest lowercase.

But words with mixed case, or multiple words in a match with each beginning with a capital, frequently lead to unexpected results.

I'm hitting it right now in a new Weir rule that corrects "link list" to "linked list" where it's changing:
> 006 Link List
into:
> 006 LinkEd list
instead of:
> 006 Linked List

It seems the whitespace is ignored but "e" is getting uppercased to "E" because the "L" is the next letter after "k".

I know I've inserted comments in code and tests over a period of months and I've mentioned this in PRs and issues before. I'll attempt to gather them here below:

- ignored test in `ChockFull`
- ignored test in `Everyday`
- ignored test in `MostNumber`
- NOTE comment in `NoLongerPronoun`
- two ignored tests in `NotOnlyInversion`
- TODO comment in `OverPlus`
- ignored test in `PluralWrongWordOfPhrase`
- TODO comment in a test in `TheTheToThatThe`
- ignored test in `ThereIsAgreement`
- ignored test in `WouldNeverHave`
- ignored test in `WrongApostrophe`
- two ignored tests in `NounCountability`
- three ignored tests in `phrase_set_corrections`

A good fix for this will probably be multi-pronged and may perhaps be a breaking change.

- It should be data driven and work with the examples mentioned in all the linters mentioned above.
- It should not break any other tests.
- It should probably have some heuristics to decide a strategy based on characters, words, etc.
- It should probably be able to return multiple suggestions rather than just one, which may be a breaking change.

Contributor guide

Open the contributing guide

Research direction

Start at replace_with_match_case and the Suggestion-generation method, then inspect the ignored tests and TODO/NOTE cases listed for the affected rules. Use those examples as regression cases, including “Link List” to “Linked list,” while checking that existing tests remain passing. Done means mixed-case and multiword corrections preserve the intended capitalization without regressions.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
tooling
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.