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

Enforce code quality standards #111

Closed
zugaldia opened this issue Jul 11, 2016 · 11 comments
Closed

Enforce code quality standards #111

zugaldia opened this issue Jul 11, 2016 · 11 comments
Assignees

Comments

@zugaldia
Copy link
Member

The main SDK is thinking of enforcing code quality standards in mapbox/mapbox-gl-native#4299. We should tag along for consistency.

@zugaldia
Copy link
Member Author

zugaldia commented Jul 12, 2016

The basic infrastructure is now in place, just use ./gradlew checkstyle.

However:

  1. We're far from compliance.
  2. It isn't yet integrated with Bitrise.
  3. I'm not sure we're using the right checkstyle.xml (currently using the one from https://github.com/checkstyle/checkstyle/blob/master/src/main/resources/google_checks.xml.

Reopening to discuss these.

@cammace
Copy link
Contributor

cammace commented Jul 13, 2016

Discovered this awesome IDEA plugin which will give you warnings right inline with the code, example:

screen shot 2016-07-13 at 2 21 39 pm

it's pretty configurable too. Still looking into all the rules in checkstyle.xml.

@cammace
Copy link
Contributor

cammace commented Jul 13, 2016

I've made my way through the massive amounts of documentation checkstyle has and figured out how to add a custom check (in PR #123). Now I'm going through and cleaning up the obvious warnings checkstyle has within the testapp. If it's something i'm not sure we should fix, i'll leave it for discussion and possible removal of that rule.

@cammace
Copy link
Contributor

cammace commented Jul 13, 2016

PR #124 includes most of the checkstyle warning fixes within the testapp only. My thoughts so far:

  • It successfully catches Hungarian notation!
  • Ordering of imports rule, I believe called THIRD_PARTY_PACKAGE goes against what Android Studio wants the order to be.
  • Android Studio uses 12 indentation but check style is wanting 8 indentation? This is causing a ton of warnings in, for example, this file.
  • Currently a rule that limits lines to a 120 character length. This is good except when you have a long LineString. This can always be moved to the strings.xml though.
  • A rule should be added to catch variables and imports not being used.
  • Checkstyle warns when a variable name is a single letter, which makes sense in most cases except here:
    screen shot 2016-07-13 at 4 11 14 pm
  • We should still look for something to make sure XML's formatted correctly as well, Checkstyle only checks Java.

@bleege
Copy link
Contributor

bleege commented Aug 11, 2016

@bleege
Copy link
Contributor

bleege commented Aug 11, 2016

Style From Mapbox Android Legacy:

https://github.com/mapbox/ma-sdk-legacy/blob/mb-pages/checks.xml

@bleege
Copy link
Contributor

bleege commented Aug 11, 2016

Square Style From Retrofit:

https://github.com/square/retrofit/blob/master/checkstyle.xml

@zugaldia
Copy link
Member Author

@mapbox/android As discussed, we're gonna with https://github.com/square/java-code-styles. I'll install and document this and ping you all when ready.

@cammace
Copy link
Contributor

cammace commented Aug 19, 2016

After setting up java-code-styles, I attempted to get sqaures checkstyle.xml file up and running in Android Studio and it was outputing quite a few errors. Did a little research and after reading this ticket, it looks like squares using Google java format instead with explicitly says:

There is no configurability as to the formatter's algorithm for formatting. This is a deliberate design decision to unify our code formatting on a single format.

Waiting on this till Monday when we can discuss this more internally.

@zugaldia
Copy link
Member Author

@cammace As discussed today, let's continue with researching https://github.com/google/google-java-format/ instead and, if it looks good, let's just adopt it.

@cammace
Copy link
Contributor

cammace commented Aug 22, 2016

Looking more into this, I setup google-java-format in Studio and quickly realized it only automates the process of reformatting the code in the file, it doesn't cause warnings/errors like checkstyle does. It also doesn't look like integration with CI would be possible.

Moving forward, I think the best route will be to continue with using the current Google checkstyle in this repo and customizing it where we see fit over the next few weeks. Next steps will be cleaning up the code so all warnings, errors are relieved and then integrating into Bitrise.

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

No branches or pull requests

3 participants