-
Notifications
You must be signed in to change notification settings - Fork 10
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
Set up checkstyle #69
Comments
I have created a pull request for this issue: |
@rishabhsethi Thanks! A few notes though:
|
Thanks for the review. |
The two things I mentioned (plus whatever you consider appropriate, if there is anything else). I.e. line width and usage of magic numbers. |
I have made the suggested changes. Although I am still not sure what more can be ignored from the checkstyle rules (I am sure many useless checks are still there). Will make changes as I become more familiar with the code base. |
I played around with the style a bit and I'd suggest going with Google's code style, which I'll update a bit once you submit a PR. The checkstyle config is here: https://github.com/checkstyle/checkstyle/blob/master/src/main/resources/google_checks.xml The thing I don't like in your PR is that it uses a different config file for each module - that's redundant and prone to introduce inconsistencies. Please use a single configuration file. Here's a guide describing how to use checkstyle in a multi-module Maven project: https://maven.apache.org/plugins/maven-checkstyle-plugin/examples/multi-module-config.html Can you update the PR? Thanks! |
Set up Checkstyle for the backend code using Maven Checkstyle Plugin.
The text was updated successfully, but these errors were encountered: