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/18 00:51:11 UTC
[GitHub] [fineract] thesmallstar opened a new pull request #906: [WIP] FINERACT-942 Remove use of printStackTrace and added checkstyle
thesmallstar opened a new pull request #906:
URL: https://github.com/apache/fineract/pull/906
Background: https://issues.apache.org/jira/browse/FINERACT-942
Requesting an early review, since there are a lot of files to change(Around 40).
Just FYI, I used "throws" keyword on some functions so as to deal with chained exceptions.
REF: #833
----------------------------------------------------------------
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 #906: [WIP] FINERACT-942 Remove use of printStackTrace and added checkstyle
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #906:
URL: https://github.com/apache/fineract/pull/906#issuecomment-631446217
Instead of "Runtime exception occurred in...", I would probably just use"problem occurred in..." because the , e) you're adding at the end automatically logs the details, incl. whether or not it was a RuntimeException or what not, so you don't need to repeat that the message. Does that help?
----------------------------------------------------------------
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 #906: [WIP] FINERACT-942 Remove use of printStackTrace and added checkstyle
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #906:
URL: https://github.com/apache/fineract/pull/906#issuecomment-630635248
@thesmallstar no in non-test it depends.. definitely never printStackTrace() but either catch(SomeException e) { LOG.error("explanation", e) -OR- (re, "chain") throw NewEception("Details", e) - but typically never BOTH log and re-throw but only either-or. HTH?
----------------------------------------------------------------
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 a change in pull request #906: [WIP] FINERACT-942 Remove use of printStackTrace and added checkstyle
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on a change in pull request #906:
URL: https://github.com/apache/fineract/pull/906#discussion_r426943870
##########
File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/AccountingScenarioIntegrationTest.java
##########
@@ -587,11 +587,8 @@ public void checkPeriodicAccrualAccountingFlow() {
this.journalEntryHelper.checkJournalEntryForAssetAccount(assetAccount, this.EXPECTED_DISBURSAL_DATE, assetAccountInitialEntry);
final String jobName = "Add Accrual Transactions";
- try {
- this.schedulerJobHelper.executeJob(jobName);
- } catch (InterruptedException e) {
- e.printStackTrace();
Review comment:
Yes, I will update the readme.
----------------------------------------------------------------
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 #906: FINERACT-942 Remove use of printStackTrace and added checkstyle
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #906:
URL: https://github.com/apache/fineract/pull/906#discussion_r428561244
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/calendar/service/CalendarUtils.java
##########
@@ -114,7 +116,7 @@ private static Date convertToiCal4JCompatibleDate(final LocalDate inputDate) {
try {
formattedDate = new Date(seedDateStr, "yyyy-MM-dd");
} catch (final ParseException e) {
- e.printStackTrace();
+ LOG.error("{}",e.getMessage());
Review comment:
Two thoughts on this this: a) your LOG.error() is wrong, as everywhere else, please:
```suggestion
LOG.error("Invalid date: " + seedDateStr, e);
```
but, b) more importantly, isn't this catch plain wrong, not required, and can just be removed?! `new Date()` does not throw ParseException - or am I missing something obvious?
----------------------------------------------------------------
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 #906: [WIP] FINERACT-942 Remove use of printStackTrace and added checkstyle
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #906:
URL: https://github.com/apache/fineract/pull/906#discussion_r426435287
##########
File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/AccountingScenarioIntegrationTest.java
##########
@@ -587,11 +587,8 @@ public void checkPeriodicAccrualAccountingFlow() {
this.journalEntryHelper.checkJournalEntryForAssetAccount(assetAccount, this.EXPECTED_DISBURSAL_DATE, assetAccountInitialEntry);
final String jobName = "Add Accrual Transactions";
- try {
- this.schedulerJobHelper.executeJob(jobName);
- } catch (InterruptedException e) {
- e.printStackTrace();
Review comment:
No, in a test, exceptions typically should not be logged but just "fall through" (propagate, with 'testXYZ() throws SomeException, Another # Exception...'), so that the test fails, if the exception happens. PS: We need to write this and more like this up into Logging Guidelines, e.g. on the README. Do you want to make a start with that in this 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] thesmallstar commented on pull request #906: FINERACT-942 Remove use of printStackTrace and added checkstyle
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #906:
URL: https://github.com/apache/fineract/pull/906#issuecomment-631747493
@vorburger this is ready for review :)
----------------------------------------------------------------
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 #906: [WIP] FINERACT-942 Remove use of printStackTrace and added checkstyle
Posted by GitBox <gi...@apache.org>.
awasum commented on pull request #906:
URL: https://github.com/apache/fineract/pull/906#issuecomment-631412012
Also seems there are some issues to be resolved as noted by @vorburger .
----------------------------------------------------------------
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 #906: [WIP] FINERACT-942 Remove use of printStackTrace and added checkstyle
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #906:
URL: https://github.com/apache/fineract/pull/906#issuecomment-630013805
The general direction LGTM here... please tag me again when it builds green for a full review.
----------------------------------------------------------------
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 #906: FINERACT-942 Remove use of printStackTrace and added checkstyle
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #906:
URL: https://github.com/apache/fineract/pull/906#discussion_r428767281
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/calendar/service/CalendarUtils.java
##########
@@ -114,7 +116,7 @@ private static Date convertToiCal4JCompatibleDate(final LocalDate inputDate) {
try {
formattedDate = new Date(seedDateStr, "yyyy-MM-dd");
} catch (final ParseException e) {
- e.printStackTrace();
+ LOG.error("{}",e.getMessage());
Review comment:
LGTM!
----------------------------------------------------------------
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 #906: [WIP] FINERACT-942 Remove use of printStackTrace and added checkstyle
Posted by GitBox <gi...@apache.org>.
awasum commented on pull request #906:
URL: https://github.com/apache/fineract/pull/906#issuecomment-631411694
Build shows that Travis passed: https://travis-ci.org/github/apache/fineract/builds/688892634
But this is still showing pending on the UI? Open and Close? or 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] thesmallstar commented on pull request #906: [WIP] FINERACT-942 Remove use of printStackTrace and added checkstyle
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #906:
URL: https://github.com/apache/fineract/pull/906#issuecomment-631027214
The build has passed on Travis I am not sure why is this still yellow?
https://travis-ci.org/github/apache/fineract/builds/688892634
----------------------------------------------------------------
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 #906: [WIP] FINERACT-942 Remove use of printStackTrace and added checkstyle
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #906:
URL: https://github.com/apache/fineract/pull/906#issuecomment-630927838
@vorburger This build should pass, in the file MultiException the printStacktrace method was overridden and a custom print-stream/writer was used for specific logging purpose, I was unsure how to tackle that? For now, I have deleted the lines(definitely needs a better approach), what do you think?
----------------------------------------------------------------
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 #906: [WIP] FINERACT-942 Remove use of printStackTrace and added checkstyle
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #906:
URL: https://github.com/apache/fineract/pull/906#discussion_r427580010
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/exception/MultiException.java
##########
@@ -97,7 +95,6 @@ public void printStackTrace(PrintWriter s) {
int i = 0;
for (Throwable e : throwables) {
s.print(++i + ".");
- e.printStackTrace(s);
Review comment:
as above
----------------------------------------------------------------
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 #906: [WIP] FINERACT-942 Remove use of printStackTrace and added checkstyle
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #906:
URL: https://github.com/apache/fineract/pull/906#discussion_r427579788
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/exception/MultiException.java
##########
@@ -87,7 +86,6 @@ public void printStackTrace(PrintStream s) {
int i = 0;
for (Throwable e : throwables) {
s.print(++i + ".");
- e.printStackTrace(s);
Review comment:
@thesmallstar this needs to be kept. It's "special", and SupressWarning is OK here (only)
----------------------------------------------------------------
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 #906: [WIP] FINERACT-942 Remove use of printStackTrace and added checkstyle
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #906:
URL: https://github.com/apache/fineract/pull/906#issuecomment-631030301
@vorburger whoops, I missed that from the last message, Updating 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] vorburger merged pull request #906: FINERACT-942 Remove use of printStackTrace and added checkstyle
Posted by GitBox <gi...@apache.org>.
vorburger merged pull request #906:
URL: https://github.com/apache/fineract/pull/906
----------------------------------------------------------------
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 #906: [WIP] FINERACT-942 Remove use of printStackTrace and added checkstyle
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #906:
URL: https://github.com/apache/fineract/pull/906#issuecomment-631446842
Thank for the quick response :D, on 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] thesmallstar commented on pull request #906: [WIP] FINERACT-942 Remove use of printStackTrace and added checkstyle
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #906:
URL: https://github.com/apache/fineract/pull/906#issuecomment-631440458
@vorburger
Just a to make sure,
LOG.error("Runtime exception occurred in importEntity function",ex);
Is this the expected explanation? If not do I need to dive deeper on what might cause the exception for each function try-catch and write an explanation?
----------------------------------------------------------------
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] xurror commented on a change in pull request #906: [WIP] FINERACT-942 Remove use of printStackTrace and added checkstyle
Posted by GitBox <gi...@apache.org>.
xurror commented on a change in pull request #906:
URL: https://github.com/apache/fineract/pull/906#discussion_r426406214
##########
File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/AccountingScenarioIntegrationTest.java
##########
@@ -587,11 +587,8 @@ public void checkPeriodicAccrualAccountingFlow() {
this.journalEntryHelper.checkJournalEntryForAssetAccount(assetAccount, this.EXPECTED_DISBURSAL_DATE, assetAccountInitialEntry);
final String jobName = "Add Accrual Transactions";
- try {
- this.schedulerJobHelper.executeJob(jobName);
- } catch (InterruptedException e) {
- e.printStackTrace();
Review comment:
I suggest you using a logger but maybe @vorburger or @awasum, @ptuomola or anyone else would have a different opinion as far as this is concerned.
It's more a convention to use here, whether checked or unchecked exceptions.
----------------------------------------------------------------
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 a change in pull request #906: FINERACT-942 Remove use of printStackTrace and added checkstyle
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on a change in pull request #906:
URL: https://github.com/apache/fineract/pull/906#discussion_r428725673
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/calendar/service/CalendarUtils.java
##########
@@ -114,7 +116,7 @@ private static Date convertToiCal4JCompatibleDate(final LocalDate inputDate) {
try {
formattedDate = new Date(seedDateStr, "yyyy-MM-dd");
} catch (final ParseException e) {
- e.printStackTrace();
+ LOG.error("{}",e.getMessage());
Review comment:
It does throw a parse exception, a possible explanation is the string received might be complete gibberish and could not be parsed as a date?
----------------------------------------------------------------
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 a change in pull request #906: FINERACT-942 Remove use of printStackTrace and added checkstyle
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on a change in pull request #906:
URL: https://github.com/apache/fineract/pull/906#discussion_r428726584
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/calendar/service/CalendarUtils.java
##########
@@ -114,7 +116,7 @@ private static Date convertToiCal4JCompatibleDate(final LocalDate inputDate) {
try {
formattedDate = new Date(seedDateStr, "yyyy-MM-dd");
} catch (final ParseException e) {
- e.printStackTrace();
+ LOG.error("{}",e.getMessage());
Review comment:
Replacing it with:
LOG.error("Invalid date: {} ", seedDateStr, e);
or spot bugs won't like 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] vorburger commented on a change in pull request #906: [WIP] FINERACT-942 Remove use of printStackTrace and added checkstyle
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #906:
URL: https://github.com/apache/fineract/pull/906#discussion_r427580230
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/exception/MultiException.java
##########
@@ -65,7 +65,6 @@ public String getMessage() {
sb.append(++i);
sb.append(". ");
Writer w = CharStreams.asWriter(sb);
- e.printStackTrace(new PrintWriter(w, true));
Review comment:
as below, must keep this as is
----------------------------------------------------------------
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 #906: [WIP] FINERACT-942 Remove use of printStackTrace and added checkstyle
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #906:
URL: https://github.com/apache/fineract/pull/906#discussion_r427580141
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/exception/MultiException.java
##########
@@ -77,7 +76,7 @@ public void printStackTrace() {
int i = 0;
for (Throwable e : throwables) {
LOG.info("{}.",++i);
- e.printStackTrace();
Review comment:
as below
----------------------------------------------------------------
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 #906: FINERACT-942 Remove use of printStackTrace and added checkstyle
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #906:
URL: https://github.com/apache/fineract/pull/906#issuecomment-631678739
@xurror can you review why the build is failing? Mostly not related to my PR(The build passed 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] thesmallstar edited a comment on pull request #906: FINERACT-942 Remove use of printStackTrace and added checkstyle
Posted by GitBox <gi...@apache.org>.
thesmallstar edited a comment on pull request #906:
URL: https://github.com/apache/fineract/pull/906#issuecomment-631678739
@xurror can you review why the build is failing? Mostly not related to my PR(The build passed locally)
Edit: Hope that the build passes after a 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] thesmallstar commented on pull request #906: [WIP] FINERACT-942 Remove use of printStackTrace and added checkstyle
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #906:
URL: https://github.com/apache/fineract/pull/906#issuecomment-630504716
@vorburger there are many instances where printStacktrace is used in a non-test file, for that case should I comment or remove the line only? Or Am I expected to follow the same approach(which turns out to be weird due to inheritance).
----------------------------------------------------------------
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