Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Detekt Baseline Warnings] FluxC Module - Resolve/Suppress Style Warnings #2490

Merged
merged 17 commits into from
Aug 10, 2022

Conversation

ParaskP7
Copy link
Contributor

@ParaskP7 ParaskP7 commented Aug 9, 2022

Parent #2479
Partially Closes: #2485

This PR resolves/suppresses all style related warnings for the fluxc module:

  1. 70 x DataClassShouldBeImmutable (Resolve: 8a96a86 + Suppress: b018434)
  2. 1 x ExplicitItLambdaParameter (Resolve: 82aaf44)
  3. 10 x ForbiddenComment (Suppress: 3b61e96 + 8b1b570)
  4. 18 x MagicNumber (Resolve: 1eb75cb + fcfcfee)
  5. 15 x MaxLineLength (Resolve: 22addfc + Suppress: d310804)
  6. 10 x ReturnCount (Resolve: 3be3c53 + Suppress: eae42a7)
  7. 1 x UnnecessaryAbstractClass (Resolve: 62f9565)
  8. 4 x UnusedPrivateMember (Resolve: 11333dd + f6e3c74 + Suppress: 6bc8592)
  9. 1 x UseCheckOrError (Resolve: dd8ac78)
  10. 2 x UtilityClassWithPublicConstructor (Resolve: 8e68338)

PS.1: I am randomly adding one engineer from the WPAndroid team and one from the WCAndroid team as the main reviewers, that is, in addition to the @wordpress-mobile/owl-team team itself, since I just want someone from these teams to sign-off on that change as well (or additionally to the 馃 team).

PS.2: I recommend reviewing this PR commit-by-commit.


To test:

  • There is nothing much to test here.
  • Verifying that all the CI checks are successful should be enough (especially the detekt check).
  • However, if you really want to be thorough, you could smoke test the example and/or the WPAndroid/WCAndroid apps to verify that everything works as expected.

PS: Please ignore the failing connected-tests check as they are unrelated to this PR. This check has been failing consistency for some time now due to tests being flaky or needing to be updated/fixed.

Description: "Data classes should mainly be used to store immutable
data. This rule assumes that they should not contain any mutable
properties."

For more info see: https://detekt.dev/docs/rules/style/
#dataclassshouldbeimmutable
Description: "Data classes should mainly be used to store immutable
data. This rule assumes that they should not contain any mutable
properties."

For more info see: https://detekt.dev/docs/rules/style/
#dataclassshouldbeimmutable
Description: "Lambda expressions are one of the core features of the
language. They often include very small chunks of code using only one
parameter. In this cases Kotlin can supply the implicit it parameter to
make code more concise. It fits most usecases, but when faced larger or
nested chunks of code, you might want to add an explicit name for the
parameter. Naming it just it is meaningless and only makes your code
misleading, especially when dealing with nested functions."

For more info see: https://detekt.dev/docs/rules/style/
#explicititlambdaparameter
Description: "This rule allows to set a list of comments which are
forbidden in the codebase and should only be used during development.
Offending code comments will then be reported."

For more info see: https://detekt.dev/docs/rules/style/#forbiddencomment
The 'ArrayList' initial capacity was explicitly set to a specific number
depending on the amount of parameters, per API call. However, this is
totally optional and not affecting the outcome of the function, nor its
performance. The default initial capacity is 10 anyway and as such,
having something less than (3, 4 or 5 in this case), even slightly more
than that (ie 20, 50 or 100), doesn't help much, performance-wise.

Description: "This rule detects and reports usages of magic numbers in
the code. Prefer defining constants with clear names describing what the
magic number means."

For more info see: https://detekt.dev/docs/rules/style/#magicnumber
Description: "This rule detects and reports usages of magic numbers in
the code. Prefer defining constants with clear names describing what the
magic number means."

For more info see: https://detekt.dev/docs/rules/style/#magicnumber
Description: "Long lines might be hard to read on smaller screens or
printouts. Additionally, having a maximum line length in the codebase
will help make the code more uniform."

For more info see: https://detekt.dev/docs/rules/style/#maxlinelength
Description: "Long lines might be hard to read on smaller screens or
printouts. Additionally, having a maximum line length in the codebase
will help make the code more uniform."

For more info see: https://detekt.dev/docs/rules/style/#maxlinelength
Description: "Having many exit points in a function can be confusing and
impacts readability of the code."

For more info see: https://detekt.dev/docs/rules/style/#returncount
Description: "Having many exit points in a function can be confusing and
impacts readability of the code."

For more info see: https://detekt.dev/docs/rules/style/#returncount
Description: "This rule inspects 'abstract' classes. In case an
'abstract class' does not have any concrete members it should be
refactored into an interface. Abstract classes which do not define any
'abstract' members should instead be refactored into concrete classes."

For more info see: https://detekt.dev/docs/rules/style/
#unnecessaryabstractclass
This also fixes the 'UnusedPrivateMember' Detekt warning for this class.
Description: "Reports unused private properties, function parameters and
functions. If these private elements are unused they should be removed.
Otherwise, this dead code can lead to confusion and potential
bugs."

For more info see: https://detekt.dev/docs/rules/style/
#unusedprivatemember
Description: "Reports unused private properties, function parameters and
functions. If these private elements are unused they should be removed.
Otherwise, this dead code can lead to confusion and potential
bugs."

For more info see: https://detekt.dev/docs/rules/style/
#unusedprivatemember
Description: "Kotlin provides a concise way to check invariants as well
as pre- and post-conditions. Prefer them instead of manually throwing an
IllegalStateException."

For more info see: https://detekt.dev/docs/rules/style/#usecheckorerror
Description: "A class which only contains utility variables and
functions with no concrete implementation can be refactored into an
'object' or a class with a non-public constructor. Furthermore, this
rule reports utility classes which are not final."

For more info see: https://detekt.dev/docs/rules/style/
#utilityclasswithpublicconstructor

Note: As part of this commit, the unused 'versionRegex' was also
removed from the 'WhatsNewAppVersionUtils' utility class.
Warning Message: "Stores must be annotated with @singleton"

For more info see: https://github.com/wordpress-mobile/
WordPress-FluxC-Android/blob/trunk/config/checkstyle.xml#L84-L88
Copy link
Contributor

@ovitrif ovitrif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work @ParaskP7 馃憦

You're the Detekt champion of the season 馃ぉ 馃 馃挴

Looks good to me, I've retried the failing connected-tests CI check 馃

@ParaskP7
Copy link
Contributor Author

馃憢 @ovitrif !

Thank you so much for the review and testing, you rock! 馃檱 馃専 馃殌

You're the Detekt champion of the season 馃ぉ 馃 馃挴

馃槄 馃槅

@ParaskP7 ParaskP7 merged commit e5cf7a2 into trunk Aug 10, 2022
@ParaskP7 ParaskP7 deleted the analysis/fluxc-style-detekt-baseline-warnings branch August 10, 2022 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resolve/Suppress Detekt Baseline Warnings - FluxC Module
2 participants