feat: add ThereOwn linter for "there own" → "their own"#3336
Conversation
Flag "there own", "they're own", and "theyre own" and suggest the possessive "their own". Includes positive and negative test cases. Registered in default config and lint group.
|
Cool. This is one that I'd been looking at recently but had taken the deep dive into. In particular I was worried about a class of false positives I expect to be common: "people there own fancy cars". I see the current code here does not attempt to avoid flagging these. There's a couple more in my feature request for this that you may or may not have seen but didn't reference: #3276 I'm guessing you made this with an AI. The AI tools I've used never include decent false positive checks until you give them a list of examples. They usually only do the kinds of negative checks I see at the bottom of the |
hippietrail
left a comment
There was a problem hiding this comment.
Sorry this needs to avoid some common false positives such as "people there own nice cars".
|
|
||
| fn match_to_lint(&self, matched_tokens: &[Token], source: &[char]) -> Option<Lint> { | ||
| let offender = matched_tokens.first()?; | ||
| let template = offender.span.get_content(source); |
There was a problem hiding this comment.
See my comment on how the PR in its current state does not try to avoid flagging false positives that I believe were pointed out in the original issue.
Skip the lint when "there" is preceded by a noun/pronoun and "own" is followed by a noun/adjective, indicating "own" is used as a verb meaning "to possess" rather than as part of "their own". Example: "people there own nice cars" is valid grammar.
Drop the followed_by_word check that required a noun/adjective after "own". Preceding nominal alone is sufficient to identify adverbial "there" + verb "own" (e.g. "companies there own the property"). Adds a test covering the determiner-after-own case.
…PR review Adds 8 tests proving the existing context guard handles the cases called out in PR review and issue Automattic#3276: - 4 positive cases pulled verbatim from issue Automattic#3276 ("there own") - 2 positive cases for "they're own" from issue Automattic#3276 - The reviewer's verbatim false-positive example ("People there own expensive cars.") - A sentence-initial "There own X" true positive guarding against the no-preceding-token edge case Brings test count from 8 to 16, satisfying AGENTS.md's "≥15 tests for new rules" guidance. All 5309 harper-core lib tests still pass.
|
Thanks for the review and for linking #3276 — I'd missed the issue, that example list is gold. Two commits after your review (8e79989, plus the new 70c4650) add a context guard that skips the lint when "there" is preceded by a noun or pronoun, since that pattern marks "there" as the adverb and "own" as the verb. 70c4650 adds 8 tests covering your example verbatim ("People there own expensive cars."), the four "there own" positive cases from #3276 (split data, customise effort lvl, modules c files, scripts create connection, silent appstore updates), two "they're own" cases (they're own command, they're own library), and a sentence-initial guard. One thing I left as-is: "they're own" / "theyre own" still always lint, since you described those as "always a possessive error before own" and I couldn't find a counter-example. Happy to add a guard there too if you have a case in mind. |
|
Thanks for the concrete example, @hippietrail. The branch already covers that class. In 70c4650 I added #[test]
fn issue_3276_false_positive_expensive_cars() {
assert_lint_count("People there own expensive cars.", ThereOwn::default(), 0);
}CLI confirmation on the branch tip: Heuristic: if the token before Happy to add |
hippietrail
left a comment
There was a problem hiding this comment.
Thanks for addressing everything!
elijah-potter
left a comment
There was a problem hiding this comment.
Looks good. Thanks!
Issues
N/A
Description
Adds a
ThereOwnlinter that catches "there own", "they're own", and "theyre own" and suggests the possessive "their own". Common mistake in informal writing.How Has This Been Tested?
Checklist