Add Checkstyle to POM with Google settings advice #4047

Merged
merged 4 commits into from Nov 1, 2016

Conversation

Projects
None yet
4 participants
Contributor

nickcmaynard commented Oct 21, 2016

PR checklist

  • Read the contribution guildelines.
  • Ran the shell/batch script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates)
  • Filed the PR against the correct branch: master for non-breaking changes and 2.3.0 branch for breaking (non-backward compatible) changes.

Description of the PR

Add Checkstyle to the Maven build, reporting noncompliance, but not enforcing style.

@nickcmaynard nickcmaynard Add Checkstyle to POM with Google settings advice
f142307
Contributor

nickcmaynard commented Oct 21, 2016

Beginnings of work on #3972.

Contributor

cbornet commented Oct 21, 2016

The code is currently indented with 4 spaces so I don't think it follows google's style.

Contributor

nickcmaynard commented Oct 21, 2016

See changes to the config XML file.

Contributor

nickcmaynard commented Oct 22, 2016

To be clear, the config has been adjusted to account for 4-space indentation.

Contributor

ePaul commented Oct 25, 2016

To be clear, the config has been adjusted to account for 4-space indentation.

Maybe also state that in the comment at the begin of the file, so people don't think this is the original one?

@nickcmaynard nickcmaynard Add note re. changes for swagger-codegen
d067aa9
Contributor

nickcmaynard commented Oct 26, 2016

Good idea - done.

@nickcmaynard nickcmaynard Use Checkstyle 6.19 - v7 removed JDK 7 runtime support
184269d
google_checkstyle.xml
@@ -1,6 +1,11 @@
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC "-//Puppy Crawl//DTD Check Configuration 1.3//EN" "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">
+<!--
+ Adjusted from original Google Checkstyle config to match requirements for swagger-codegen
+ See detailed comments next to changes
@ePaul

ePaul Oct 26, 2016

Contributor

Hmm, I don't understand this sentence. Maybe better "Following comment is from the original file retrieved from ..." (and include the URL of the source)?

(Also, please use some punctuation at the end of your sentences.)

@nickcmaynard

nickcmaynard Oct 26, 2016

Contributor

I shall add a link to the original file. However, I shan't be paying a great deal of attention to comments on grammar in a comment on a configuration file where it's patently obvious what's going on.

@nickcmaynard nickcmaynard Add a link to the original config
6db783c
Contributor

nickcmaynard commented Oct 27, 2016

@wing328 Any thoughts on merging this?

Contributor

ePaul commented Oct 28, 2016

Looks good to me. 👍

wing328 added this to the v2.2.2 milestone Nov 1, 2016

Member

wing328 commented Nov 1, 2016

@nickcmaynard I did some tests and found the warnings (which are expected).

Thanks for the contribution.

wing328 merged commit 0c1b1aa into swagger-api:master Nov 1, 2016

3 checks passed

Shippable Run 892 status is SUCCESS.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

nickcmaynard deleted the nickcmaynard:checkstyle branch Nov 1, 2016

@davidgri davidgri pushed a commit to davidgri/swagger-codegen that referenced this pull request May 11, 2017

@nickcmaynard @wing328 nickcmaynard + wing328 Add Checkstyle to POM with Google settings advice (#4047)
* Add Checkstyle to POM with Google settings advice

* Add note re. changes for swagger-codegen

* Use Checkstyle 6.19 - v7 removed JDK 7 runtime support

* Add a link to the original config
15b9556
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment