You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by GitBox <gi...@apache.org> on 2020/05/08 09:24:22 UTC
[GitHub] [fineract] percyashu opened a new pull request #830: FINERACT-822 enable and enforce Error Prone
percyashu opened a new pull request #830:
URL: https://github.com/apache/fineract/pull/830
## Description
Describe the changes made and why they were made. Ignore if these details are present on the associated Jira ticket: https://issues.apache.org/jira/browse/FINERACT-822
## Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
- [ ] Commit message starts with the issue number from https://issues.apache.org/jira/projects/FINERACT/. Ex: FINERACT-646 Pockets API.
- [ ] Coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions have been followed.
- [ ] API documentation at https://github.com/apache/fineract/blob/develop/api-docs/apiLive.htm has been updated with details of any API changes.
- [ ] Integration tests have been created/updated for verifying the changes made.
- [ ] All Integrations tests are passing with the new commits.
- [ ] Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the list.)
Our guidelines for code reviews is at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] awasum commented on a change in pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
awasum commented on a change in pull request #830:
URL: https://github.com/apache/fineract/pull/830#discussion_r430886565
##########
File path: fineract-provider/build.gradle
##########
@@ -278,6 +280,86 @@ configurations {
tasks.withType(JavaCompile) {
options.compilerArgs += ["-Xlint:unchecked","-Xlint:cast","-Werror"] // TODO FINERACT-959 (gradually) enable -Xlint:all (see "javac -help -X")
options.deprecation = true
+ options.errorprone {
+ disableWarningsInGeneratedCode = true
+ //Todo Remove necessary checks from this list
+ disable("UnusedVariable",
Review comment:
I see that you have added these so you can enable them and fix gradually rather than fixing the errors at once? Make sure you look at the issues rules you are disabling so that we dont miss catching some critical problems in the system.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] awasum commented on pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
awasum commented on pull request #830:
URL: https://github.com/apache/fineract/pull/830#issuecomment-635454264
> some initial review feedback.. hope I'm not "too harsh" ?
>
> @awasum perhaps we could alternative reviewing this one - see my feedback, and next round you check if @percyashu has taken what I'm raising into account, and I'll only re-check it after you LGTM it, would that work for you?
>
> /Cc @thesmallstar FYI given that you also work on code quality, perhaps this review feedback interests you.
Sounds like a good plan.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] percyashu commented on a change in pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
percyashu commented on a change in pull request #830:
URL: https://github.com/apache/fineract/pull/830#discussion_r429565613
##########
File path: fineract-provider/build.gradle
##########
@@ -276,7 +278,7 @@ configurations {
}
tasks.withType(JavaCompile) {
- options.compilerArgs += ["-Xlint:unchecked","-Xlint:cast","-Werror"] // TODO FINERACT-959 (gradually) enable -Xlint:all (see "javac -help -X")
+ options.compilerArgs += ["-Xlint:unchecked","-Xlint:cast"] // TODO FINERACT-959 (gradually) enable -Xlint:all (see "javac -help -X")
Review comment:
@vorburger thanks for the link I will configure error prone to help with the 1000 warnings.
##########
File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/StaffTest.java
##########
@@ -99,9 +101,10 @@ public void testStaffCreateMaxNameLength() {
map.put("firstname", Utils.randomNameGenerator("michael_", 42));
map.put("lastname", Utils.randomNameGenerator("Doe_", 46));
- StaffHelper.createStaffWithJson(requestSpec, responseSpec, new Gson().toJson(map));
+ StaffHelper.createStaffWithJson(requestSpec, responseSpecForValidationError, new Gson().toJson(map));
Review comment:
Ok will do those in seperate Pr.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #830:
URL: https://github.com/apache/fineract/pull/830#issuecomment-629551303
> Now with #859 merged, warnings cause build to fail. Fixing them as they cause build fail now
@percyashu FYI that's done in #893
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] thesmallstar commented on pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #830:
URL: https://github.com/apache/fineract/pull/830#issuecomment-629458798
If I am not wrong, @percyashu has this planned for his summer in GSoC?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] percyashu commented on a change in pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
percyashu commented on a change in pull request #830:
URL: https://github.com/apache/fineract/pull/830#discussion_r422529704
##########
File path: fineract-provider/src/main/java/org/apache/fineract/notification/domain/Notification.java
##########
@@ -90,7 +90,7 @@ public Long getActor() {
}
public void setActor(Long actor) {
- this.actorId = actorId;
+ this.actorId = actor;
}
Review comment:
@vorburger concerning org.apache.fineract.notification.domain.Notification.setActor(Long) error prone picked this up locally
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] awasum commented on a change in pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
awasum commented on a change in pull request #830:
URL: https://github.com/apache/fineract/pull/830#discussion_r431971297
##########
File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/StaffTest.java
##########
@@ -91,6 +92,8 @@ public void testStaffCreateValidationError() {
StaffHelper.createStaffWithJson(requestSpec, responseSpecForValidationError, new Gson().toJson(map));
}
+ @Test
+ @Ignore //TODO FINERACT-1005
Review comment:
Create an issue for this ignored test so someone should fix it in the near future.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] awasum commented on pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
awasum commented on pull request #830:
URL: https://github.com/apache/fineract/pull/830#issuecomment-634619667
/rebase
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] awasum commented on pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
awasum commented on pull request #830:
URL: https://github.com/apache/fineract/pull/830#issuecomment-634455171
While you are working on this...keep an eye out for the conflicts that develop as a result of other PRs and fix them.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] percyashu commented on pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
percyashu commented on pull request #830:
URL: https://github.com/apache/fineract/pull/830#issuecomment-629474295
Am going to address it here @vorburger. Start to remove the warnings from compileJava.
Yeah @thesmallstar i have plan to do this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #830:
URL: https://github.com/apache/fineract/pull/830#issuecomment-629403805
> @vorburger Sure was think the same thing.
Ok, great, and.... were you going to address that here? :smile: Or would you like someone else to have a closer look and see if they can raise a follow-up PR which takes yours but removes all the new ignores and fixes those problems properly? Maybe this is something that could you interest you @thesmallstar? (You don't have to, I just thought you may like this; this is related to the "code quality" theme overall, but has nothing directly to do with your Checkstyle related efforts.)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] percyashu commented on a change in pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
percyashu commented on a change in pull request #830:
URL: https://github.com/apache/fineract/pull/830#discussion_r431565330
##########
File path: fineract-provider/build.gradle
##########
@@ -278,6 +280,86 @@ configurations {
tasks.withType(JavaCompile) {
options.compilerArgs += ["-Xlint:unchecked","-Xlint:cast","-Werror"] // TODO FINERACT-959 (gradually) enable -Xlint:all (see "javac -help -X")
options.deprecation = true
+ options.errorprone {
+ disableWarningsInGeneratedCode = true
+ //Todo Remove necessary checks from this list
+ disable("UnusedVariable",
Review comment:
Yep these are default warning checks and will be removed from this list in separate PR
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] percyashu commented on a change in pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
percyashu commented on a change in pull request #830:
URL: https://github.com/apache/fineract/pull/830#discussion_r431971357
##########
File path: fineract-provider/build.gradle
##########
@@ -278,6 +280,88 @@ configurations {
tasks.withType(JavaCompile) {
options.compilerArgs += ["-Xlint:unchecked","-Xlint:cast","-Werror"] // TODO FINERACT-959 (gradually) enable -Xlint:all (see "javac -help -X")
options.deprecation = true
+ options.errorprone {
+ disableWarningsInGeneratedCode = true
+ //Todo Remove necessary checks from this list
+ disable("UnusedVariable",
+ "VariableNameSameAsType",
+ "InconsistentCapitalization",
+ "UnnecessaryParentheses",
+ "ClassCanBeStatic",
+ "MissingCasesInEnumSwitch",
+ "CompareToZero",
+ "OperatorPrecedence",
+ "MixedMutabilityReturnType",
+ "SameNameButDifferent",
+ "EqualsGetClass",
+ "ReferenceEquality",
+ "StringSplitter",
+ "MissingOverride",
+ "BigDecimalEquals",
+ "ImmutableEnumChecker",
+ "EqualsUnsafeCast",
+ "AssertEqualsArgumentOrderChecker",
+ "BoxedPrimitiveEquality",
+ "BadImport",
+ "UnusedMethod",
+ "ModifiedButNotUsed",
+ "ArgumentSelectionDefectChecker",
+ "JodaPlusMinusLong",
+ "TypeParameterUnusedInFormals",
+ "DoubleBraceInitialization",
+ "UnusedNestedClass",
+ "UndefinedEquals",
+ "JdkObsolete",
+ "OperatorPrecedence",
+ "NarrowingCompoundAssignment"
+ )
+ //TODO gradually enable these checks
+ error( "DefaultCharset",
Review comment:
Yeah
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] awasum commented on pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
awasum commented on pull request #830:
URL: https://github.com/apache/fineract/pull/830#issuecomment-635455452
@percyashu If you have addressed a review feedback by @vorburger , just click on "Resolve Conversation" so its clear.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger merged pull request #830: FINERACT-822 Integrate Error Prone to Fineract
Posted by GitBox <gi...@apache.org>.
vorburger merged pull request #830:
URL: https://github.com/apache/fineract/pull/830
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #830:
URL: https://github.com/apache/fineract/pull/830#issuecomment-626325244
@percyashu I've just merged #846 so you're unblocked now - I look forward to reviewing this, once you manage to make it pass the build!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] percyashu commented on a change in pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
percyashu commented on a change in pull request #830:
URL: https://github.com/apache/fineract/pull/830#discussion_r429565765
##########
File path: fineract-provider/src/main/java/org/apache/fineract/interoperation/util/MathUtil.java
##########
@@ -87,6 +87,7 @@ public static Long abs(Long value) {
/** @return calculates minimum of the two values considering null values
* @param notNull if true then null parameter is omitted, otherwise returns null */
+ @SuppressWarnings("NullTernary")
Review comment:
Will look more into those remaining two remaining errors and provide if it is necessary to use the @SuppressWarnings.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a change in pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #830:
URL: https://github.com/apache/fineract/pull/830#discussion_r429559026
##########
File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/StaffTest.java
##########
@@ -99,9 +101,10 @@ public void testStaffCreateMaxNameLength() {
map.put("firstname", Utils.randomNameGenerator("michael_", 42));
map.put("lastname", Utils.randomNameGenerator("Doe_", 46));
- StaffHelper.createStaffWithJson(requestSpec, responseSpec, new Gson().toJson(map));
+ StaffHelper.createStaffWithJson(requestSpec, responseSpecForValidationError, new Gson().toJson(map));
Review comment:
this seems a bit suspicious to me - so this test if failing, but you are just making the response expect a validation error? Shouldn't we... fix this test, if we re-enable it, instead of cheating to get the test to pass? Re-enabling these tests could be done in a separate PR before this one, if you like (but don't have to).
##########
File path: fineract-provider/build.gradle
##########
@@ -276,7 +278,7 @@ configurations {
}
tasks.withType(JavaCompile) {
- options.compilerArgs += ["-Xlint:unchecked","-Xlint:cast","-Werror"] // TODO FINERACT-959 (gradually) enable -Xlint:all (see "javac -help -X")
+ options.compilerArgs += ["-Xlint:unchecked","-Xlint:cast"] // TODO FINERACT-959 (gradually) enable -Xlint:all (see "javac -help -X")
Review comment:
wait waaaaat... why are we regressing on FINERACT-959 to progress with FINERACT-822? That doesn't seem right, to me - and worth a discussion... :smiling_imp: I suspect that you may be doing this because Error Prone gives warnings? IMHO we should either fix them all, as part of this - or change EP's config to shut up about particular ones which are obviously wrong (similar to what I have done in another project, have you seen the link in FINERACT-822)... How many are there?
##########
File path: fineract-provider/build.gradle
##########
@@ -435,6 +437,9 @@ dependencies {
tomcat "org.apache.tomcat:tomcat:9.0.34@zip"
}
+dependencies {
+ errorprone "com.google.errorprone:error_prone_core:2.3.3"
Review comment:
```suggestion
errorprone "com.google.errorprone:error_prone_core:2.3.4"
```
##########
File path: fineract-provider/src/main/java/org/apache/fineract/interoperation/util/MathUtil.java
##########
@@ -87,6 +87,7 @@ public static Long abs(Long value) {
/** @return calculates minimum of the two values considering null values
* @param notNull if true then null parameter is omitted, otherwise returns null */
+ @SuppressWarnings("NullTernary")
Review comment:
whoa whoa - slow down, the point is to FIX all problems - not to introduce a tool that tells us about problems, and then just make the tool shut up! I have not looked closely, but just a quick glance at https://errorprone.info/bugpattern/NullTernary makes it seem to me like this is trying to telly us about a real bug? As above, if it's definitely a false positive, then put a detailed analysis explanation in an inline comment...
##########
File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/StaffTest.java
##########
@@ -183,13 +186,17 @@ public void testStaffUpdate() {
Assert.assertEquals(mobileNo, changes.get("mobileNo"));
}
+ @Test
+ @Ignore
Review comment:
can you raise a JIRA for the failure and then link it here using `@Ignore // TODO FINERACT-123` syntax?
##########
File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/variableinstallments/VariableInstallmentsFlatHelper.java
##########
@@ -201,6 +201,7 @@ private static ArrayList createModifyMap(String date) {
return toReturn;
}
+ @SuppressWarnings("MisusedWeekYear")
Review comment:
this seems suspicious - are you sure suppressing this is correct, and not actually hiding a real bug? (I haven't had a closer look, I'm hoping you'll do that...) If you are completely sure, then each `@SuppressWarnings` should have an inline comment with a reasonably detailed explanation why that is the right thing to do.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] percyashu commented on a change in pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
percyashu commented on a change in pull request #830:
URL: https://github.com/apache/fineract/pull/830#discussion_r426582785
##########
File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/StaffTest.java
##########
@@ -183,13 +185,15 @@ public void testStaffUpdate() {
Assert.assertEquals(mobileNo, changes.get("mobileNo"));
}
+ @SuppressWarnings("JUnit4TestNotRun")
public void testStaffUpdateLongExternalIdError() {
final HashMap<String, Object> map = new HashMap<>();
map.put("externalId", Utils.randomStringGenerator("EXT", 98));
StaffHelper.updateStaff(requestSpec, responseSpecForValidationError, 1, map);
}
Review comment:
@vorburger or @awasum this looks like a test any idea why they are missing @Test.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] awasum commented on pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
awasum commented on pull request #830:
URL: https://github.com/apache/fineract/pull/830#issuecomment-634619926
If rebase does not fix these conflicts @percyashu , please try to resolve them manually.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a change in pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #830:
URL: https://github.com/apache/fineract/pull/830#discussion_r427647609
##########
File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/StaffTest.java
##########
@@ -183,13 +185,15 @@ public void testStaffUpdate() {
Assert.assertEquals(mobileNo, changes.get("mobileNo"));
}
+ @SuppressWarnings("JUnit4TestNotRun")
public void testStaffUpdateLongExternalIdError() {
final HashMap<String, Object> map = new HashMap<>();
map.put("externalId", Utils.randomStringGenerator("EXT", 98));
StaffHelper.updateStaff(requestSpec, responseSpecForValidationError, 1, map);
}
Review comment:
@percyashu I suppose someone just "forgot" to add `@Test` ?
What I suggest you do is do add `@Test` for all these cases (instead of `@SuppressWarnings("JUnit4TestNotRun")`), and try running them. If they fail, slap an `@Ignore` on those that do fail - and then subsequently but in a later separate PR see if you can fix them (or someone else can do that, as part of future work to review all ignored tests). Sounds like a plan?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] percyashu commented on a change in pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
percyashu commented on a change in pull request #830:
URL: https://github.com/apache/fineract/pull/830#discussion_r426038837
##########
File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/variableinstallments/VariableInstallmentsFlatHelper.java
##########
@@ -201,6 +201,7 @@ private static ArrayList createModifyMap(String date) {
return toReturn;
}
+ @SuppressWarnings("MisusedWeekYear")
Review comment:
@vorburger not sure the of the right format that was supposed to be on methods I added @SuppressWarnings("MisuedWeekYear") please see https://errorprone.info/bugpattern/MisusedWeekYear
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] percyashu commented on pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
percyashu commented on pull request #830:
URL: https://github.com/apache/fineract/pull/830#issuecomment-629507038
Am going to address it here @vorburger. Started to remove the warnings from compileJava.
Yeah @thesmallstar i have plan to do this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] awasum commented on a change in pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
awasum commented on a change in pull request #830:
URL: https://github.com/apache/fineract/pull/830#discussion_r431969800
##########
File path: fineract-provider/build.gradle
##########
@@ -278,6 +280,86 @@ configurations {
tasks.withType(JavaCompile) {
options.compilerArgs += ["-Xlint:unchecked","-Xlint:cast","-Werror"] // TODO FINERACT-959 (gradually) enable -Xlint:all (see "javac -help -X")
options.deprecation = true
+ options.errorprone {
+ disableWarningsInGeneratedCode = true
+ //Todo Remove necessary checks from this list
+ disable("UnusedVariable",
Review comment:
Lets not forget to enable some of the important rules you have disable here in future PRs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] awasum commented on a change in pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
awasum commented on a change in pull request #830:
URL: https://github.com/apache/fineract/pull/830#discussion_r431971297
##########
File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/StaffTest.java
##########
@@ -91,6 +92,8 @@ public void testStaffCreateValidationError() {
StaffHelper.createStaffWithJson(requestSpec, responseSpecForValidationError, new Gson().toJson(map));
}
+ @Test
+ @Ignore //TODO FINERACT-1005
Review comment:
Create an issue for this ingored test so someone should fix it in the near future.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] percyashu commented on a change in pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
percyashu commented on a change in pull request #830:
URL: https://github.com/apache/fineract/pull/830#discussion_r431971891
##########
File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/StaffTest.java
##########
@@ -91,6 +92,8 @@ public void testStaffCreateValidationError() {
StaffHelper.createStaffWithJson(requestSpec, responseSpecForValidationError, new Gson().toJson(map));
}
+ @Test
+ @Ignore //TODO FINERACT-1005
Review comment:
I have created an issue for it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] percyashu commented on pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
percyashu commented on pull request #830:
URL: https://github.com/apache/fineract/pull/830#issuecomment-635079934
@awasum LGTY?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #830:
URL: https://github.com/apache/fineract/pull/830#issuecomment-632572284
@percyashu are you on this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] percyashu commented on pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
percyashu commented on pull request #830:
URL: https://github.com/apache/fineract/pull/830#issuecomment-630144788
@vorburger I meant with "-Werror" errorprone compiler warnings are now errors.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] awasum commented on a change in pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
awasum commented on a change in pull request #830:
URL: https://github.com/apache/fineract/pull/830#discussion_r431970882
##########
File path: fineract-provider/build.gradle
##########
@@ -278,6 +280,88 @@ configurations {
tasks.withType(JavaCompile) {
options.compilerArgs += ["-Xlint:unchecked","-Xlint:cast","-Werror"] // TODO FINERACT-959 (gradually) enable -Xlint:all (see "javac -help -X")
options.deprecation = true
+ options.errorprone {
+ disableWarningsInGeneratedCode = true
+ //Todo Remove necessary checks from this list
+ disable("UnusedVariable",
+ "VariableNameSameAsType",
+ "InconsistentCapitalization",
+ "UnnecessaryParentheses",
+ "ClassCanBeStatic",
+ "MissingCasesInEnumSwitch",
+ "CompareToZero",
+ "OperatorPrecedence",
+ "MixedMutabilityReturnType",
+ "SameNameButDifferent",
+ "EqualsGetClass",
+ "ReferenceEquality",
+ "StringSplitter",
+ "MissingOverride",
+ "BigDecimalEquals",
+ "ImmutableEnumChecker",
+ "EqualsUnsafeCast",
+ "AssertEqualsArgumentOrderChecker",
+ "BoxedPrimitiveEquality",
+ "BadImport",
+ "UnusedMethod",
+ "ModifiedButNotUsed",
+ "ArgumentSelectionDefectChecker",
+ "JodaPlusMinusLong",
+ "TypeParameterUnusedInFormals",
+ "DoubleBraceInitialization",
+ "UnusedNestedClass",
+ "UndefinedEquals",
+ "JdkObsolete",
+ "OperatorPrecedence",
+ "NarrowingCompoundAssignment"
+ )
+ //TODO gradually enable these checks
+ error( "DefaultCharset",
Review comment:
if I understand you correctly, you want to chop this up into multiple PRs by enabling/uncommenting one or a few rules at a time and fixing them?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] percyashu commented on pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
percyashu commented on pull request #830:
URL: https://github.com/apache/fineract/pull/830#issuecomment-626403265
@vorburger build passes now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a change in pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #830:
URL: https://github.com/apache/fineract/pull/830#discussion_r422531950
##########
File path: fineract-provider/src/main/java/org/apache/fineract/notification/domain/Notification.java
##########
@@ -90,7 +90,7 @@ public Long getActor() {
}
public void setActor(Long actor) {
- this.actorId = actorId;
+ this.actorId = actor;
}
Review comment:
@percyashu nice! FYI I've also fixed this in #858 ...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] percyashu commented on pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
percyashu commented on pull request #830:
URL: https://github.com/apache/fineract/pull/830#issuecomment-626399264
@vorburger there are a lot of warning and the errors that occurred I modified the ones I understood and some like the MisusedWeekYear just added a suppress warning since not very sure. Hence build should no longer fail because of errors picked up by error prone.
The build seems to now fail due to not able to download some jars. I will check and see why this is failing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] percyashu commented on a change in pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
percyashu commented on a change in pull request #830:
URL: https://github.com/apache/fineract/pull/830#discussion_r429565630
##########
File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/StaffTest.java
##########
@@ -183,13 +186,17 @@ public void testStaffUpdate() {
Assert.assertEquals(mobileNo, changes.get("mobileNo"));
}
+ @Test
+ @Ignore
Review comment:
OK will do
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] awasum commented on pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
awasum commented on pull request #830:
URL: https://github.com/apache/fineract/pull/830#issuecomment-631391149
Seems there is a conflict here. @percyashu . Want to address that? Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] awasum commented on pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
awasum commented on pull request #830:
URL: https://github.com/apache/fineract/pull/830#issuecomment-625924696
@percyashu ..Have u being able to check the logs and see why this is failing? (I have not checked)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] percyashu commented on pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
percyashu commented on pull request #830:
URL: https://github.com/apache/fineract/pull/830#issuecomment-629515861
Now with https://github.com/apache/fineract/pull/859 merged, warnings cause build to fail. Fixing them as they cause build fail now
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] percyashu commented on pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
percyashu commented on pull request #830:
URL: https://github.com/apache/fineract/pull/830#issuecomment-632855640
@vorburger yes i am on this. Will check why this is failling
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] percyashu commented on pull request #830: FINERACT-822 enable and enforce Error Prone
Posted by GitBox <gi...@apache.org>.
percyashu commented on pull request #830:
URL: https://github.com/apache/fineract/pull/830#issuecomment-626217558
@awasum It is due to Travis java version. Error prone has some compatibility issues with java 12. Once the pull request to change the version is merge I will run checks again.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org