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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve pre-existing codenarc style violations #160

Merged
merged 26 commits into from
May 8, 2019

Conversation

fractalwrench
Copy link
Contributor

Goal

The CodeNarc static analysis tool was added in #158 - this PR addresses pre-existing violations, where they have been simple to address. In total 16 violations have been left in the codebase with a view to addressing them as separate tickets at a later date.

Changeset

  • Reduced violation count to 16, adjusted baseline accordingly
  • Added @CompileStatic where changes made this possible, to gain the benefits of type-safety
  • Suppressed the following overzealous checks by default:
    • InstanceOf
    • ClassJavadoc
    • ExplicitArrayListInstantiation
    • ExplicitHashSetInstantiation
    • ExplicitHashMapInstantiation
    • JavaPackageAccess
    • UnnecessaryElseStatement
    • UnnecessaryToString

Existing violations were addressed in both the main + test sourcesets. The following made up the majority of the issues:

  • Use of def rather than an explicit type such as String
  • Incorrect ordering of imports
  • Unnecessary use of semicolon (not required in Groovy)
  • Duplicate string literals
  • Not using property access syntax (e.g. foo instead of getFoo())
  • Unnecessary use of return keyword (not required in Groovy)

Tests

  • Ran existing unit tests
  • Ran existing mazerunner scenarios
  • Ran ./gradlew assemble in an example Android app using Gradle 5.4.0 and AGP 3.3.0 and confirmed upload of mapping files was triggered

@@ -1,5 +1,9 @@
package com.bugsnag.android.gradle

import groovy.transform.CompileStatic

@SuppressWarnings('DuplicateStringLiteral')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are duplicate strings in this enum constructor, but they represent different concepts and shouldn't be grouped together into one variable.

statusCode = response.statusLine.statusCode
HttpEntity entity = response.entity
responseEntity = EntityUtils.toString(entity, "utf-8")
} catch (IOException | ParseException e) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One of the inspections was to make the caught exception less generic - I've done this where the code documents clearly what exception types can be thrown.

@@ -50,7 +52,7 @@ class BugsnagUploadNdkTask extends BugsnagMultiPartUploadTask {
project.logger.lifecycle("Found shared object file (${arch}) ${sharedObject}")
sharedObjectFound = true

File outputFile = createSymbolsForSharedObject(sharedObject, arch)
File outputFile = generateSymbolsForSharedObject(sharedObject, arch)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

create* indicates a factory method in groovy - this has been renamed to avoid a warning.

Copy link
Contributor

@simonbowring simonbowring left a comment

Choose a reason for hiding this comment

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

LGTM

@fractalwrench fractalwrench merged commit 12eb223 into master May 8, 2019
@fractalwrench fractalwrench deleted the address-inspections branch May 8, 2019 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants