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/03/14 09:42:44 UTC
[GitHub] [fineract] thesmallstar opened a new pull request #733: Removed
Console output and added checkstyle module
thesmallstar opened a new pull request #733: Removed Console output and added checkstyle module
URL: https://github.com/apache/fineract/pull/733
## Description
There were many TODOs, to get started and learn the correct method I thought this would be the easiest one :P
The build is passing but I would require following confirmations:
1. Is this the correct approach, should I continue the same way for other TODOs.
2. Since we are removing the console outputs, was it expected to log it?
3. In Listener.java the build failed if the method msg.getText() is not invoked, current implementation seems like a temporary fix, what is the correct way?
4. Removed Console outputs and replaced them with a new line(blank line) any suggestions on this?
## 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
With regards,
Apache Git Services
[GitHub] [fineract] vorburger commented on issue #733: Removed Console
output and added checkstyle module
Posted by GitBox <gi...@apache.org>.
vorburger commented on issue #733: Removed Console output and added checkstyle module
URL: https://github.com/apache/fineract/pull/733#issuecomment-602308713
> What do think? Do you think this is a good and worthy Idea?
absolutely, please go for it. (I'm not sure it's a full GSoC project, but I would say start with this; I expect you'll probably be able to complete it sooner than you think - and then you can look for more work!)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [fineract] thesmallstar commented on issue #733: Removed Console
output and added checkstyle module
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on issue #733: Removed Console output and added checkstyle module
URL: https://github.com/apache/fineract/pull/733#issuecomment-612667080
@awasum fixed a part of it, will be done soon :)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [fineract] vorburger commented on issue #733: Removed Console
output and added checkstyle module
Posted by GitBox <gi...@apache.org>.
vorburger commented on issue #733: Removed Console output and added checkstyle module
URL: https://github.com/apache/fineract/pull/733#issuecomment-599190176
@thesmallstar rebase this, and we'll review it when build passed!
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [fineract] thesmallstar commented on issue #733: Removed Console
output and added checkstyle module
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on issue #733: Removed Console output and added checkstyle module
URL: https://github.com/apache/fineract/pull/733#issuecomment-599196548
@vorburger the build looks good 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
With regards,
Apache Git Services
[GitHub] [fineract] thesmallstar commented on issue #733: Removed Console
output and added checkstyle module
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on issue #733: Removed Console output and added checkstyle module
URL: https://github.com/apache/fineract/pull/733#issuecomment-599222846
@vorburger I have replaced System.out with LOG statements, but Spotbug is bothered with log performance and suggesting some changes, Is there a way to solve this? than going individually through all 200-250 lines?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [fineract] awasum commented on issue #733: Removed Console output
and added checkstyle module
Posted by GitBox <gi...@apache.org>.
awasum commented on issue #733: Removed Console output and added checkstyle module
URL: https://github.com/apache/fineract/pull/733#issuecomment-612579612
Status 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
With regards,
Apache Git Services
[GitHub] [fineract] thesmallstar edited a comment on issue #733: Removed
Console output and added checkstyle module
Posted by GitBox <gi...@apache.org>.
thesmallstar edited a comment on issue #733: Removed Console output and added checkstyle module
URL: https://github.com/apache/fineract/pull/733#issuecomment-602083160
@vorburger I am planning to add This into my GSOC proposal, Adding all check-style modules and working on removing concatenated SQL strings(using SQL builder) and solving other security issues that are confirmed by the plugin, along with solving some pending bugs on Jira.
Considering the amount of work, I thought this is a would-be a worthy candidate I have also discussed the same with @awasum who liked the idea.
What do think? Do you think this is a good and worthy Idea?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [fineract] awasum commented on issue #733: Removed Console output
and added checkstyle module
Posted by GitBox <gi...@apache.org>.
awasum commented on issue #733: Removed Console output and added checkstyle module
URL: https://github.com/apache/fineract/pull/733#issuecomment-599170600
Try to fix root cause of build generally failing... Will you like to help? Please check flyway mismatch numbering problem with latest merge...
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [fineract] vorburger commented on issue #733: Removed Console
output and added checkstyle module
Posted by GitBox <gi...@apache.org>.
vorburger commented on issue #733: Removed Console output and added checkstyle module
URL: https://github.com/apache/fineract/pull/733#issuecomment-599232398
> @vorburger I have replaced System.out with LOG statements, but Spotbug is bothered with log performance and suggesting some changes,
Yeah, that's due to the JUST introduced #716, see [FINERACT-696](https://issues.apache.org/jira/browse/FINERACT-696) for background.
> Is there a way to solve this rather than going individually through all 200-250 lines?
I pulled this locally and noticed there are 270 violations... too many to manually fix up? :smile: Do a bunch every day, instead of all together?
Alternatives I can think of could be to configure Checkstyle to not run this particular check on src/test and only src/main, but I think that will be a PITA to configure (you would need separate configuration files, and somehow tell the Gradle plugin to run different configs for different dirs; I'm not sure that's possible), and also confusing for future developers, so I wouldn't do that and keep it simple.
In terms of best use of time, what I would probably do if I were you is maybe pause this one, and first move on to gradually activating other Checkstyle checks, and then come back to this one later / at the end.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [fineract] thesmallstar commented on issue #733: Removed Console
output and added checkstyle module
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on issue #733: Removed Console output and added checkstyle module
URL: https://github.com/apache/fineract/pull/733#issuecomment-599200662
> I didn't understand what you meant here.
@vorburger , I just realized that was a repeated question, I meant if the method implemented in this PR is correct( turns out it is ).
Replacing with LOG statements, thank you for the 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
With regards,
Apache Git Services
[GitHub] [fineract] thesmallstar commented on issue #733: Removed Console
output and added checkstyle module
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on issue #733: Removed Console output and added checkstyle module
URL: https://github.com/apache/fineract/pull/733#issuecomment-602083160
@vorburger I am planning to add This into my GSOC proposal, Adding all check-style modules and working on removing concatenated SQL strings(using SQL builder) and solving other security issues that are confirmed by the plugin.
Considering the amount of work, I thought this is a would-be a worthy candidate I have also discussed the same with @awasum who liked the idea.
What do think? Do you think this is a good and worthy Idea?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [fineract] thesmallstar commented on issue #733: Removed Console
output and added checkstyle module
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on issue #733: Removed Console output and added checkstyle module
URL: https://github.com/apache/fineract/pull/733#issuecomment-599036109
The build is failing probably #732 will fix 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
With regards,
Apache Git Services
[GitHub] [fineract] thesmallstar edited a comment on issue #733: Removed
Console output and added checkstyle module
Posted by GitBox <gi...@apache.org>.
thesmallstar edited a comment on issue #733: Removed Console output and added checkstyle module
URL: https://github.com/apache/fineract/pull/733#issuecomment-599222846
@vorburger I have replaced System.out with LOG statements, but Spotbug is bothered with log performance and suggesting some changes, Is there a way to solve this rather than going individually through all 200-250 lines?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [fineract] thesmallstar commented on issue #733: Removed Console
output and added checkstyle module
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on issue #733: Removed Console output and added checkstyle module
URL: https://github.com/apache/fineract/pull/733#issuecomment-599233186
@vorburger agreed, actually, I manually added those logger variables to 93 files(each one being unique with class name) and hoped for the reward of merged PR 😸
I would go for editing this daily then, should be done in a week without being bored :P
-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
With regards,
Apache Git Services