-
-
Notifications
You must be signed in to change notification settings - Fork 794
add better documentation for the LongParameterList ignoreAnnotated #2714
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2714 +/- ##
============================================
- Coverage 80.52% 80.45% -0.08%
+ Complexity 2322 2303 -19
============================================
Files 378 378
Lines 6944 6954 +10
Branches 1258 1262 +4
============================================
+ Hits 5592 5595 +3
- Misses 725 729 +4
- Partials 627 630 +3 Continue to review full report at Codecov.
|
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.
Thanks for improving the docs and taking the time to submit a PR!
The docs are automatically generated from the rule description.
The corresponding rule needs to be updated first.
Thanks...that wasn't obvious :-) |
* annotation class names (default: `[]`) | ||
* annotation class names (default: `[]`); (value is a comma separated string with the annotation class names); | ||
* the most common case is for Dagger constructors that are annotated with @Inject which would be specified | ||
* with `Inject` (quotes are required) |
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 think that the `
are not needed. They should not at least. And instead of use csv you can use [Inject, Module, Suppress]
or, if you prefer ['Inject', 'Module', 'Suppress']
. Or yml even allow you to do this:
ignoreAnnotated:
- Inject
- Module
- Suppress
Anyway, I agree that we should improve our documentation to say what's the expected value for each parameter.
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.
Hmm..in other parts of our config file, we do stuff like this:
exceptions: 'IllegalArgumentException,IllegalStateException,IOException'
and
excludes: "/test/,/androidTest/,/*.Test.kt,/.Spec.kt,**/.Spek.kt"
The docs probably need a section on specifying lists of values that this config param description could link to. I didn't realize it was just a regular YAML parser or that you could specify strings w/o quotes in YAML :-)
Is there a preferred way of filling out these multivalue fields?
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.
@BraisGabin recently added the support for the normal yaml list syntax for rule properties (which is more readable imo).
We should totally encourage users to use this syntax.
Comma separated values will still be supported for a very long time.
From Kotlinlang slack discussion:
https://kotlinlang.slack.com/archives/C88E12QH4/p1589836335126300
Wasn't obvious that ignoreAnnotated should include a CSV list of class names. Updated documentation to mention that and added an example.