[jira] [Commented] (FINERACT-942) Make Checkstyle detect bad logging anti-patterns (and fix problems found)
[ https://issues.apache.org/jira/browse/FINERACT-942?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17184845#comment-17184845 ] Michael Vorburger commented on FINERACT-942: FINERACT-1131 created re. IllegalCatch and resolving this for 1.4.0! > Make Checkstyle detect bad logging anti-patterns (and fix problems found) > - > > Key: FINERACT-942 > URL: https://issues.apache.org/jira/browse/FINERACT-942 > Project: Apache Fineract > Issue Type: Improvement >Reporter: Michael Vorburger >Assignee: Manthan Surkar >Priority: Major > Fix For: 1.5.0 > > > A particularly useful part of FINERACT-821, helping to detect real errors in > Fineract and not just formating, would be to enable the following in > fineract-provider/config/checkstyle/checkstyle.xml, and fix any problems that > this finds: > 1. RegexpSinglelineJava / printStackTrace > 2. IllegalCatch, IllegalThrows, MutableException, > AvoidHidingCauseExceptionCheck > 3. > [EmptyCatchBlock|https://checkstyle.sourceforge.io/config_blocks.html#EmptyCatchBlock] > Nota bene that we already have FINERACT-696, but this is complementary to > that. > One thing this should detect is e.g. the bad wrong emtpy > catch(MessagingException e) in EmailMessageJobEmailServiceImpl. > It may lead to more of FINERACT-932, which would be a Good Thing. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (FINERACT-942) Make Checkstyle detect bad logging anti-patterns (and fix problems found)
[ https://issues.apache.org/jira/browse/FINERACT-942?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17184807#comment-17184807 ] Michael Vorburger commented on FINERACT-942: Oh wait I didn't actually read the previous comments here, and kind of forgot where we left this off... perhaps the best thing to do here for clarity would be to change the Fix Version of this to 1.4.0 and close it, and open a issue for 1.5.0 specifically for the (only) remaining IllegalCatch? > Make Checkstyle detect bad logging anti-patterns (and fix problems found) > - > > Key: FINERACT-942 > URL: https://issues.apache.org/jira/browse/FINERACT-942 > Project: Apache Fineract > Issue Type: Improvement >Reporter: Michael Vorburger >Assignee: Manthan Surkar >Priority: Major > Fix For: 1.5.0 > > > A particularly useful part of FINERACT-821, helping to detect real errors in > Fineract and not just formating, would be to enable the following in > fineract-provider/config/checkstyle/checkstyle.xml, and fix any problems that > this finds: > 1. RegexpSinglelineJava / printStackTrace > 2. IllegalCatch, IllegalThrows, MutableException, > AvoidHidingCauseExceptionCheck > 3. > [EmptyCatchBlock|https://checkstyle.sourceforge.io/config_blocks.html#EmptyCatchBlock] > Nota bene that we already have FINERACT-696, but this is complementary to > that. > One thing this should detect is e.g. the bad wrong emtpy > catch(MessagingException e) in EmailMessageJobEmailServiceImpl. > It may lead to more of FINERACT-932, which would be a Good Thing. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (FINERACT-942) Make Checkstyle detect bad logging anti-patterns (and fix problems found)
[ https://issues.apache.org/jira/browse/FINERACT-942?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17184805#comment-17184805 ] Michael Vorburger commented on FINERACT-942: I would strongly support changing the Fix Version of this back from 1.5.0 to 1.4.0, IFF you can raise the PR for the missing 1 checkstyle ASAP, for both develop, and then cherry-pick backport it to 1.4.0 still. > Make Checkstyle detect bad logging anti-patterns (and fix problems found) > - > > Key: FINERACT-942 > URL: https://issues.apache.org/jira/browse/FINERACT-942 > Project: Apache Fineract > Issue Type: Improvement >Reporter: Michael Vorburger >Assignee: Manthan Surkar >Priority: Major > Fix For: 1.5.0 > > > A particularly useful part of FINERACT-821, helping to detect real errors in > Fineract and not just formating, would be to enable the following in > fineract-provider/config/checkstyle/checkstyle.xml, and fix any problems that > this finds: > 1. RegexpSinglelineJava / printStackTrace > 2. IllegalCatch, IllegalThrows, MutableException, > AvoidHidingCauseExceptionCheck > 3. > [EmptyCatchBlock|https://checkstyle.sourceforge.io/config_blocks.html#EmptyCatchBlock] > Nota bene that we already have FINERACT-696, but this is complementary to > that. > One thing this should detect is e.g. the bad wrong emtpy > catch(MessagingException e) in EmailMessageJobEmailServiceImpl. > It may lead to more of FINERACT-932, which would be a Good Thing. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (FINERACT-942) Make Checkstyle detect bad logging anti-patterns (and fix problems found)
[ https://issues.apache.org/jira/browse/FINERACT-942?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17181639#comment-17181639 ] Manthan Surkar commented on FINERACT-942: - only 1 checkstyle is left I would say it is better to add this in 1.4.0 and make an issue for the remaining. Are you okay with it? > Make Checkstyle detect bad logging anti-patterns (and fix problems found) > - > > Key: FINERACT-942 > URL: https://issues.apache.org/jira/browse/FINERACT-942 > Project: Apache Fineract > Issue Type: Improvement >Reporter: Michael Vorburger >Assignee: Manthan Surkar >Priority: Major > Fix For: 1.4.0 > > > A particularly useful part of FINERACT-821, helping to detect real errors in > Fineract and not just formating, would be to enable the following in > fineract-provider/config/checkstyle/checkstyle.xml, and fix any problems that > this finds: > 1. RegexpSinglelineJava / printStackTrace > 2. IllegalCatch, IllegalThrows, MutableException, > AvoidHidingCauseExceptionCheck > 3. > [EmptyCatchBlock|https://checkstyle.sourceforge.io/config_blocks.html#EmptyCatchBlock] > Nota bene that we already have FINERACT-696, but this is complementary to > that. > One thing this should detect is e.g. the bad wrong emtpy > catch(MessagingException e) in EmailMessageJobEmailServiceImpl. > It may lead to more of FINERACT-932, which would be a Good Thing. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (FINERACT-942) Make Checkstyle detect bad logging anti-patterns (and fix problems found)
[ https://issues.apache.org/jira/browse/FINERACT-942?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17181224#comment-17181224 ] Awasum Yannick commented on FINERACT-942: - [~Manthan]. Whats left here? Maybe push this to 1.5.0 release? Or do we create issues for the remaining rules and close this one as fixed in 1.4.0 ( I prefer this one as many PRs have been merged under this). > Make Checkstyle detect bad logging anti-patterns (and fix problems found) > - > > Key: FINERACT-942 > URL: https://issues.apache.org/jira/browse/FINERACT-942 > Project: Apache Fineract > Issue Type: Improvement >Reporter: Michael Vorburger >Assignee: Manthan Surkar >Priority: Major > Fix For: 1.4.0 > > > A particularly useful part of FINERACT-821, helping to detect real errors in > Fineract and not just formating, would be to enable the following in > fineract-provider/config/checkstyle/checkstyle.xml, and fix any problems that > this finds: > 1. RegexpSinglelineJava / printStackTrace > 2. IllegalCatch, IllegalThrows, MutableException, > AvoidHidingCauseExceptionCheck > 3. > [EmptyCatchBlock|https://checkstyle.sourceforge.io/config_blocks.html#EmptyCatchBlock] > Nota bene that we already have FINERACT-696, but this is complementary to > that. > One thing this should detect is e.g. the bad wrong emtpy > catch(MessagingException e) in EmailMessageJobEmailServiceImpl. > It may lead to more of FINERACT-932, which would be a Good Thing. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (FINERACT-942) Make Checkstyle detect bad logging anti-patterns (and fix problems found)
[ https://issues.apache.org/jira/browse/FINERACT-942?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17164959#comment-17164959 ] Manthan Surkar commented on FINERACT-942: - [~vorburger] sure, 1. https://github.com/apache/fineract/blob/b61957acb20ede94bedd05edc95a505bc59ba513/fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/HookIntegrationTest.java#L79-L92 2. https://github.com/apache/fineract/blob/b61957acb20ede94bedd05edc95a505bc59ba513/fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/HookIntegrationTest.java#L93-L100 *3. * https://github.com/apache/fineract/blob/c529fdf58c335adb4dd938b0551f7e355e9eb56d/fineract-provider/src/main/java/org/apache/fineract/batch/command/internal/DisburseLoanCommandStrategy.java#L81-L93 and https://github.com/apache/fineract/blob/c529fdf58c335adb4dd938b0551f7e355e9eb56d/fineract-provider/src/main/java/org/apache/fineract/infrastructure/campaigns/sms/service/SmsCampaignDomainServiceImpl.java#L233-L235 *4.* https://github.com/apache/fineract/blob/2323450e71d1508e6ff218d48b856aefa04542d0/fineract-provider/src/main/java/org/apache/fineract/infrastructure/bulkimport/importhandler/client/ClientEntityImportHandler.java#L194-L228 > Make Checkstyle detect bad logging anti-patterns (and fix problems found) > - > > Key: FINERACT-942 > URL: https://issues.apache.org/jira/browse/FINERACT-942 > Project: Apache Fineract > Issue Type: Improvement >Reporter: Michael Vorburger >Assignee: Manthan Surkar >Priority: Major > Fix For: 1.4.0 > > > A particularly useful part of FINERACT-821, helping to detect real errors in > Fineract and not just formating, would be to enable the following in > fineract-provider/config/checkstyle/checkstyle.xml, and fix any problems that > this finds: > 1. RegexpSinglelineJava / printStackTrace > 2. IllegalCatch, IllegalThrows, MutableException, > AvoidHidingCauseExceptionCheck > 3. > [EmptyCatchBlock|https://checkstyle.sourceforge.io/config_blocks.html#EmptyCatchBlock] > Nota bene that we already have FINERACT-696, but this is complementary to > that. > One thing this should detect is e.g. the bad wrong emtpy > catch(MessagingException e) in EmailMessageJobEmailServiceImpl. > It may lead to more of FINERACT-932, which would be a Good Thing. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (FINERACT-942) Make Checkstyle detect bad logging anti-patterns (and fix problems found)
[ https://issues.apache.org/jira/browse/FINERACT-942?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17163129#comment-17163129 ] Michael Vorburger commented on FINERACT-942: [~Manthan] can yuo point to an example of existing code where I can look at what you describe above? Give a link to GitHub, or class name. > Make Checkstyle detect bad logging anti-patterns (and fix problems found) > - > > Key: FINERACT-942 > URL: https://issues.apache.org/jira/browse/FINERACT-942 > Project: Apache Fineract > Issue Type: Improvement >Reporter: Michael Vorburger >Assignee: Manthan Surkar >Priority: Major > Fix For: 1.4.0 > > > A particularly useful part of FINERACT-821, helping to detect real errors in > Fineract and not just formating, would be to enable the following in > fineract-provider/config/checkstyle/checkstyle.xml, and fix any problems that > this finds: > 1. RegexpSinglelineJava / printStackTrace > 2. IllegalCatch, IllegalThrows, MutableException, > AvoidHidingCauseExceptionCheck > 3. > [EmptyCatchBlock|https://checkstyle.sourceforge.io/config_blocks.html#EmptyCatchBlock] > Nota bene that we already have FINERACT-696, but this is complementary to > that. > One thing this should detect is e.g. the bad wrong emtpy > catch(MessagingException e) in EmailMessageJobEmailServiceImpl. > It may lead to more of FINERACT-932, which would be a Good Thing. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (FINERACT-942) Make Checkstyle detect bad logging anti-patterns (and fix problems found)
[ https://issues.apache.org/jira/browse/FINERACT-942?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17161551#comment-17161551 ] Manthan Surkar commented on FINERACT-942: - [~vorburger] The illegal catch checkstyle is the last one left, I will need some guidance before I proceed: 1. general exceptions are used to timeout request, like eg: loop: do some request if Exception wait for 3 sec and goto loop else Exit 2. Use it for no reason and just safety, and removing the try-catch block won't affect the code/cause any error. 3. A runtime exception is caught and logged 4. When an operation is to be performed on say 10 entities and we don't want to stop if an entity 5 has a runtime exception, it is caught. TBH I don't know how to tackle any of these situations with confidence. IMO, in 2,3 we should remove the logging completely. 1,4 we should suppress the warning, WDTY? > Make Checkstyle detect bad logging anti-patterns (and fix problems found) > - > > Key: FINERACT-942 > URL: https://issues.apache.org/jira/browse/FINERACT-942 > Project: Apache Fineract > Issue Type: Improvement >Reporter: Michael Vorburger >Assignee: Manthan Surkar >Priority: Major > Fix For: 1.4.0 > > > A particularly useful part of FINERACT-821, helping to detect real errors in > Fineract and not just formating, would be to enable the following in > fineract-provider/config/checkstyle/checkstyle.xml, and fix any problems that > this finds: > 1. RegexpSinglelineJava / printStackTrace > 2. IllegalCatch, IllegalThrows, MutableException, > AvoidHidingCauseExceptionCheck > 3. > [EmptyCatchBlock|https://checkstyle.sourceforge.io/config_blocks.html#EmptyCatchBlock] > Nota bene that we already have FINERACT-696, but this is complementary to > that. > One thing this should detect is e.g. the bad wrong emtpy > catch(MessagingException e) in EmailMessageJobEmailServiceImpl. > It may lead to more of FINERACT-932, which would be a Good Thing. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (FINERACT-942) Make Checkstyle detect bad logging anti-patterns (and fix problems found)
[ https://issues.apache.org/jira/browse/FINERACT-942?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17141128#comment-17141128 ] Michael Vorburger commented on FINERACT-942: I had a quick look at what's left here, and noticed that [~Manthan] you actually made good progress here.. if I'm not mistaken, what is now left here is: * AvoidHidingCauseException which we are discussing in https://github.com/apache/fineract/pull/1079 * IllegalCatch, of which there are unfortunately quite a lot (about x80 or so; a handful in integration test, more in real code) If I find the time, I may raise a PR which won't completely fix the IllegalCatch, but can perhaps help to better illustrate how to deal with it where. > Make Checkstyle detect bad logging anti-patterns (and fix problems found) > - > > Key: FINERACT-942 > URL: https://issues.apache.org/jira/browse/FINERACT-942 > Project: Apache Fineract > Issue Type: Improvement >Reporter: Michael Vorburger >Assignee: Manthan Surkar >Priority: Major > Fix For: 1.4.0 > > > A particularly useful part of FINERACT-821, helping to detect real errors in > Fineract and not just formating, would be to enable the following in > fineract-provider/config/checkstyle/checkstyle.xml, and fix any problems that > this finds: > 1. RegexpSinglelineJava / printStackTrace > 2. IllegalCatch, IllegalThrows, MutableException, > AvoidHidingCauseExceptionCheck > 3. > [EmptyCatchBlock|https://checkstyle.sourceforge.io/config_blocks.html#EmptyCatchBlock] > Nota bene that we already have FINERACT-696, but this is complementary to > that. > One thing this should detect is e.g. the bad wrong emtpy > catch(MessagingException e) in EmailMessageJobEmailServiceImpl. > It may lead to more of FINERACT-932, which would be a Good Thing. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (FINERACT-942) Make Checkstyle detect bad logging anti-patterns (and fix problems found)
[ https://issues.apache.org/jira/browse/FINERACT-942?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17113032#comment-17113032 ] Michael Vorburger commented on FINERACT-942: FTR: [https://github.com/apache/fineract/pull/912] will add (initial) Error Logging Guidelines to the README. > Make Checkstyle detect bad logging anti-patterns (and fix problems found) > - > > Key: FINERACT-942 > URL: https://issues.apache.org/jira/browse/FINERACT-942 > Project: Apache Fineract > Issue Type: Improvement >Reporter: Michael Vorburger >Assignee: Manthan Surkar >Priority: Major > Fix For: 1.4.0 > > > A particularly useful part of FINERACT-821, helping to detect real errors in > Fineract and not just formating, would be to enable the following in > fineract-provider/config/checkstyle/checkstyle.xml, and fix any problems that > this finds: > 1. RegexpSinglelineJava / printStackTrace > 2. IllegalCatch, IllegalThrows, MutableException, > AvoidHidingCauseExceptionCheck > 3. > [EmptyCatchBlock|https://checkstyle.sourceforge.io/config_blocks.html#EmptyCatchBlock] > Nota bene that we already have FINERACT-696, but this is complementary to > that. > One thing this should detect is e.g. the bad wrong emtpy > catch(MessagingException e) in EmailMessageJobEmailServiceImpl. > It may lead to more of FINERACT-932, which would be a Good Thing. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (FINERACT-942) Make Checkstyle detect bad logging anti-patterns (and fix problems found)
[ https://issues.apache.org/jira/browse/FINERACT-942?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17102589#comment-17102589 ] Michael Vorburger commented on FINERACT-942: FYI XBRLBuilderTest is covered in https://github.com/apache/fineract/pull/833 > Make Checkstyle detect bad logging anti-patterns (and fix problems found) > - > > Key: FINERACT-942 > URL: https://issues.apache.org/jira/browse/FINERACT-942 > Project: Apache Fineract > Issue Type: Improvement >Reporter: Michael Vorburger >Assignee: Manthan Surkar >Priority: Major > > A particularly useful part of FINERACT-821, helping to detect real errors in > Fineract and not just formating, would be to enable the following in > fineract-provider/config/checkstyle/checkstyle.xml, and fix any problems that > this finds: > 1. RegexpSinglelineJava / printStackTrace > 2. IllegalCatch, IllegalThrows, MutableException, > AvoidHidingCauseExceptionCheck > 3. > [EmptyCatchBlock|https://checkstyle.sourceforge.io/config_blocks.html#EmptyCatchBlock] > Nota bene that we already have FINERACT-696, but this is complementary to > that. > One thing this should detect is e.g. the bad wrong emtpy > catch(MessagingException e) in EmailMessageJobEmailServiceImpl. > It may lead to more of FINERACT-932, which would be a Good Thing. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (FINERACT-942) Make Checkstyle detect bad logging anti-patterns (and fix problems found)
[ https://issues.apache.org/jira/browse/FINERACT-942?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17102576#comment-17102576 ] Manthan Surkar commented on FINERACT-942: - [~vorburger] Yes sure, I with start with this and with the two pending PRs(which need minor changes). > Make Checkstyle detect bad logging anti-patterns (and fix problems found) > - > > Key: FINERACT-942 > URL: https://issues.apache.org/jira/browse/FINERACT-942 > Project: Apache Fineract > Issue Type: Improvement >Reporter: Michael Vorburger >Priority: Major > > A particularly useful part of FINERACT-821, helping to detect real errors in > Fineract and not just formating, would be to enable the following in > fineract-provider/config/checkstyle/checkstyle.xml, and fix any problems that > this finds: > 1. RegexpSinglelineJava / printStackTrace > 2. IllegalCatch, IllegalThrows, MutableException, > AvoidHidingCauseExceptionCheck > 3. > [EmptyCatchBlock|https://checkstyle.sourceforge.io/config_blocks.html#EmptyCatchBlock] > Nota bene that we already have FINERACT-696, but this is complementary to > that. > One thing this should detect is e.g. the bad wrong emtpy > catch(MessagingException e) in EmailMessageJobEmailServiceImpl. > It may lead to more of FINERACT-932, which would be a Good Thing. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (FINERACT-942) Make Checkstyle detect bad logging anti-patterns (and fix problems found)
[ https://issues.apache.org/jira/browse/FINERACT-942?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17102565#comment-17102565 ] Michael Vorburger commented on FINERACT-942: [~Manthan] do you want to pick this up as one of your first tasks, when you start for GSOC, next week? > Make Checkstyle detect bad logging anti-patterns (and fix problems found) > - > > Key: FINERACT-942 > URL: https://issues.apache.org/jira/browse/FINERACT-942 > Project: Apache Fineract > Issue Type: Improvement >Reporter: Michael Vorburger >Priority: Major > > A particularly useful part of FINERACT-821, helping to detect real errors in > Fineract and not just formating, would be to enable the following in > fineract-provider/config/checkstyle/checkstyle.xml, and fix any problems that > this finds: > 1. RegexpSinglelineJava / printStackTrace > 2. IllegalCatch, IllegalThrows, MutableException, > AvoidHidingCauseExceptionCheck > 3. > [EmptyCatchBlock|https://checkstyle.sourceforge.io/config_blocks.html#EmptyCatchBlock] > Nota bene that we already have FINERACT-696, but this is complementary to > that. > One thing this should detect is e.g. the bad wrong emtpy > catch(MessagingException e) in EmailMessageJobEmailServiceImpl. > It may lead to more of FINERACT-932, which would be a Good Thing. -- This message was sent by Atlassian Jira (v8.3.4#803005)
