-
Notifications
You must be signed in to change notification settings - Fork 203
feat(core): wrote IveGerund
rule
#1860
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I've looking
usage is endemic, others exist too
|
||
impl Default for IveGerund { | ||
fn default() -> Self { | ||
let expr = SequenceExpr::aco("I've") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"I/you/we/they have looking" can also be found
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And "has looking"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been thinking about the best way to handle contractions in the system generally for weeks. Currently they have no annotation in dictionary.dict
. They probably call for something like a vec/slice of WordMetadata
per "word" or such. In the meantime: I/we/you/they+have/had & he/she/it+has/had would be the full pattern.
|
||
impl Default for IveGerund { | ||
fn default() -> Self { | ||
let expr = SequenceExpr::aco("I've") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this one might be more general about using simple quote or tick or back tick for contraction.
But people might disable a rule about contraction, so here this should be detected anyway
|
||
impl Default for IveGerund { | ||
fn default() -> Self { | ||
let expr = SequenceExpr::aco("I've") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've
, they've
and you've
can be found
https://github.com/search?q=%2F%5B%5Ei%5D%27ve+looking%2F&type=code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably all the 'd
variants too for past perfect.
|
||
use super::{ExprLinter, Lint, LintKind, Suggestion}; | ||
|
||
pub struct IveGerund { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A gerund is an -ing form of a verb acting as a noun. This is actually a present participle, which is the -ing form of a verb in compound tenses (or adjectives which doesn't concern us here). And it's likely to apply to I've, I'd, we've, we'd, you've, you'd, he's, he'd, she's, she'd, it's, it'd, they've, and they'd.
The normal names for the compound tenses are present perfect
"have eaten" and past perfect
"had eaten". So we're dealing with a mixup of a perfect tense
and a present participle
. While gerund
is "too nounish" we could chose one of the "verbish" common names for the -ing form: continuous
or progressive
. So maybe PerfectContinuous
or PerfectProgressive
would be good names for a broader version of the linter?
} | ||
|
||
fn description(&self) -> &'static str { | ||
"Detects the ungrammatical pattern `I've` followed directly by a gerund (e.g., `I've looking`) and suggests either the present progressive (`I'm …`) or the present perfect progressive (`I've been …`)." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely avoid the word gerund
in the message and definition. That would be a grammar checker spreading bad grammar (-: continuous
, progressive
, or -ing form
. You've nailed the other terminology!
assert_good_and_bad_suggestions( | ||
"I've looking into it.", | ||
IveGerund::default(), | ||
&["I'm looking into it.", "I've been looking into it."], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
|
||
use super::{ExprLinter, Lint, LintKind, Suggestion}; | ||
|
||
pub struct IvePerfectProgressive { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While the code is fine the name if the rule is a bit misleading.
It now supports:
- other pronouns than
I
- other verb contraction than
've
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking it doesn't really matter in the code but I think you're right because it's also going to be the name of an option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's my concern
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicking
@@ -58,6 +58,7 @@ use super::in_on_the_cards::InOnTheCards; | |||
use super::inflected_verb_after_to::InflectedVerbAfterTo; | |||
use super::its_contraction::ItsContraction; | |||
use super::its_possessive::ItsPossessive; | |||
use super::progressive_needs_be::ProgressiveNeedsBe; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be moved aside pronoun_*
Here I feel the rule is added at the wrong place, assuming they are sorted alphabeticaly
@@ -444,6 +445,7 @@ impl LintGroup { | |||
insert_struct_rule!(HowTo, true); | |||
insert_expr_rule!(HyphenateNumberDay, true); | |||
insert_expr_rule!(IAmAgreement, true); | |||
insert_expr_rule!(ProgressiveNeedsBe, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sane remark about the position among the list
@@ -57,6 +57,7 @@ mod it_is; | |||
mod it_would_be; | |||
mod its_contraction; | |||
mod its_possessive; | |||
mod progressive_needs_be; | |||
mod left_right_hand; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
@@ -185,6 +186,7 @@ pub use inflected_verb_after_to::InflectedVerbAfterTo; | |||
pub use initialism_linter::InitialismLinter; | |||
pub use its_contraction::ItsContraction; | |||
pub use its_possessive::ItsPossessive; | |||
pub use progressive_needs_be::ProgressiveNeedsBe; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
Issues
#1753
Description
Adds a rule for addressing the grammatical error outlined in #1753
How Has This Been Tested?
Additional unit tests.
Checklist