fix(lint): add 'side' to ignore list for effect/affect rule (fix issue #2958)#2983
fix(lint): add 'side' to ignore list for effect/affect rule (fix issue #2958)#2983estefafdez wants to merge 2 commits intoAutomattic:masterfrom
Conversation
Add 'side' as an exception to prevent false positive for the common term "side effect" in the noun-verb confusion lint rule. Also adds a test case to verify the fix.
|
Hey thanks for the PR! I would say this one needs a bunch of tests since "side effect" and "side affect" are both legit English (same with "side effects" and "side affects"). Let me do a quick Google to find some examples where "affect(s)" is correct:
It is at least 10x more common as a mistake, but it's too common to be marginal as legit usage. Some are even ambiguous. Sorry I haven't looked at the PR yet so apologies if this is handled already. I just thought I'd share these thoughts with you quickly. |
Hey @hippietrail, thanks for the quick feedback! You're absolutely right — "side affect(s)" is legitimate in many contexts, so we definitely need solid test coverage to avoid false positives. I'd be happy to add more tests. Could you share more examples of cases where "side affect(s)" is correct so I can include them? That way we can make sure the rule only triggers when it's clearly a mistake and not on valid usage. Thanks again for taking the time to look into this! :) looking forward to collaborating for the first time in this project! |
|
Thanks for the feedback again @hippietrail ! I've added 6 new tests to cover the cases you mentioned. Tests that verify
Tests that verify legitimate verb uses of
The verb cases pass cleanly because Let me know if you have more examples to add :) |
elijah-potter
left a comment
There was a problem hiding this comment.
This looks good enough to merge. I'll approve it.
But, I have one question. Be honest, are you a bot? Forgive me but your code and your responses are giving "Claude". No offense intended.
Hey there @elijah-potter , no, I'm not 😂 I was leaning rust for a few months and I love your app, so I wanted to keep practicing and contribute to your Repo. You can check my profile if you want. And thanks for the approval! Looking forward to keep contributing to this project if you let me! |
|
Okay. I appreciate your work so far. It's quite good and I hope to hear more from you. @hippietrail and I were discussing another recent contributor who seems to be an OpenClaw bot. I suppose I just had it on the brain. My deepest apologies! |
No problem at all! As I said, looking forward to keep contributing to open source projects that I used every day to make them better 💪 |
I was also getting the same bot vibe Elijah was getting so I was about to write advice in the way I'd instruct a bot. But since we know you're human it's easier (-: I don't have examples to hand. What I do is use my Google-fu - bots typically can't do actual Google queries which makes it hard to automate. What I do is start with all the valid inflections and restrict the domain to GitHub:
For one like this where the false positives are fewer than one in ten I just go through all the findings page by page to see which hits are not misspellings of "side effect(s)". If I notice any patterns, like what following word types might be when it's legit but not when it's a spello, then I add to the query. For instance when not a mistake "affect" is a transitive verb, so it can take an object, which means a noun, pronoun, or noun phrase. So I might add all the object pronouns separated by
Sometimes I widen the search. Since Harper is targeting coders I start with tech/coding sites. So I'll add If I'm still looking for more, or haven't hit one of the pronouns or one verb inflection, etc. then I widen to
Happy hunting! |
Perfect, thank you for the explanation. As those are my first contribution, I tried to get small/concrete issues easy to resolve (as I'm also a junior in rust) but I will check the Repo and try to understand better the code and the way you work just to make better PRs next 😊 thanks again for the code review and the insight and thank you for doing such an amazing job with the app! |
Oh I just spotted that I forgot the essential double quotes in my Google examples. Fixed above. Even though I've been coding for decades I still feel like a junior in Rust at just over a year into it. |
Issues
Fixes #2958
Description
The
EffectToAffectlint rule was incorrectly suggesting "side affect" instead of leaving "side effect" unchanged."Side" is POS-tagged as
NOUN, which matched the rule's preceding context check and caused "effect" to be flagged as a verb form needing correction. The fix adds"side"to the existing list of words that suppress the lint (alongside"take","takes", etc.), since "side effect" is always a compound noun.Changes:
harper-core/src/linting/noun_verb_confusion/effect_affect/effect_to_affect.rs: added"side"to the preceding-word exclusion listharper-core/src/linting/noun_verb_confusion/mod.rs: added a regression test using the exact example from the issueDemo
Before:
"I forgot to test the side effect that users are deleted when clearing data."→ Harper suggests changing "effect" to "affect" ❌After: no lint triggered ✅
How Has This Been Tested?
Added a unit test
no_change_side_effectinmod.rsthat asserts 0 lints are produced for the sentence from the issue report.Checklist