-
Notifications
You must be signed in to change notification settings - Fork 235
feat: more fast → faster etc. #2263
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
elijah-potter
left a comment
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 like these changes for the most part, but I think there's a better way to filter lints out of the lint description check.
| let lints_to_check = LintGroup::new_curated(FstDictionary::curated(), Dialect::American); | ||
|
|
||
| let mut enforcer_config = LintGroupConfig::new_curated(); | ||
| enforcer_config.set_rule_enabled("MoreAdjective", false); |
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 don't love the idea of disabling specific rules from this test. Our goal here is to make sure our descriptions are clean. If we want to ignore this rule becuase it's more of a stylistic choice, rather than a grammatical error, I think we should be ignoring specific LintKinds instead.
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.
That's totally fair of course. But one problem is that it often feels more like fixing an actual grammatical error, as in "more big" or "more long", whereas it seems like a stylistic choice for words like "funner" or "oftener".
It might be best solved by a new annotation flag or two for adjectives?
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.
That's totally fair of course. But one problem is that it often feels more like fixing an actual grammatical error, as in "more big" or "more long", whereas it seems like a stylistic choice for words like "funner" or "oftener".
It might be best solved by a new annotation flag or two for adjectives?
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.
That's totally fair of course. But one problem is that it often feels more like fixing an actual grammatical error, as in "more big" or "more long", whereas it seems like a stylistic choice for words like "funner" or "oftener".
It might be best solved by a new annotation flag or two for adjectives?
Issues
Resolves #1509
Description
Flags "more/most " when the adjective has an inflected comparative/superlative form.
Note that this is not correcting an error and is not even always an improvement since native English speakers don't prefer some inflected adjective forms and they can seem clunky, awkward, or even wrong.
Because of this I've modified the
lint_descriptions_are_cleantest to turn off this one test. In the future it will likely make sense to replace this special case with an API etc. when there are other suggestions which won't mean that a description isn't clean.I made
lint_descriptions_are_cleaninlint_group.rseasier to follow while I was at it by renaming variables and adding a nice comment.While I was figuring that out, I rearranged the methods of
LintGroupto group all the constructors together at the top before the other methods.This linter's own
messagelets the user know it's not an error.I also had to modify some dictionary entries that used the
>annotation to make agent nouns from verbs ("time" → "timer" etc.) since they also assign the adjective property, resulting in false positives.How Has This Been Tested?
Unit tests should cover all the variants and known false positives. Most are made up but false positives come from GitHub.
Checklist