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/07/05 21:40:20 UTC
[GitHub] [fineract] awasum opened a new pull request #1155: FINERACT-1079: Run spotlessApply before compileJava task
awasum opened a new pull request #1155:
URL: https://github.com/apache/fineract/pull/1155
## Description
Developers consistently have problems manually running the spotlessApply task after coding changes... See: https://github.com/apache/fineract/pull/1151 and https://github.com/apache/fineract/pull/1148
maybe we should just run spotlessApply just before compilation and skip the tasks to check for spotless failures? Or we run both of them: Run spotlessApply before spotlessCheck (seems wasteful). Is this a good idea? I wonder?
## 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 fineract-provider/src/main/resources/static/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 pull request #1155: FINERACT-1079: Run spotlessApply before compileJava task
Posted by GitBox <gi...@apache.org>.
awasum commented on pull request #1155:
URL: https://github.com/apache/fineract/pull/1155#issuecomment-654025146
Initially, I wanted something that will run just before the build or bootRun tasks.. But it looks like one needs to look for a way to do this immediately before committing.
----------------------------------------------------------------
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] ptuomola commented on pull request #1155: FINERACT-1079: Run spotlessApply before compileJava task
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1155:
URL: https://github.com/apache/fineract/pull/1155#issuecomment-654017489
@awasum @xurror @thesmallstar @vorburger Just one concern: doesn't this mean that you can then check in code that doesn't pass the Spotless formatting standards, and get that to pass the build anyway?
In other words: if I now commit code that is completely wrongly formatted, when Travis runs the build, it does spotlessApply so the build passes - but the **fixed code is never committed into Github**: so if I look at the source code on Github (or clone/branch the repository), it is still incorrectly formatted?
This to me would seem to defeat the whole purpose of Spotless: it should ensure that the code in Github is correctly formatted, not that the code that is compiled is formatted correctly. The compiler doesn't care about code formatting...
Also this means that the code everyone is running is actually different from what is checked into Github. So if you e.g. look at a stack trace from someone's build and compare the line numbers with source in Github - then the line numbers will not match, as spotlessApply may have changed them. Also this means that any source code you write is automatically modified by spotless without you getting a chance to review whether the changes are correct.
Unless I'm missing something here, then this does not sound like a good idea to me...
----------------------------------------------------------------
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 #1155: FINERACT-1079: Run spotlessApply before compileJava task
Posted by GitBox <gi...@apache.org>.
awasum commented on pull request #1155:
URL: https://github.com/apache/fineract/pull/1155#issuecomment-653943316
I made a local violation and it was fixed..Please can you pull this locally and also test and let us know if it 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 #1155: FINERACT-1079: Run spotlessApply before compileJava task
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #1155:
URL: https://github.com/apache/fineract/pull/1155#issuecomment-657890216
#1158
----------------------------------------------------------------
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 merged pull request #1155: FINERACT-1079: Run spotlessApply before compileJava task
Posted by GitBox <gi...@apache.org>.
xurror merged pull request #1155:
URL: https://github.com/apache/fineract/pull/1155
----------------------------------------------------------------
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 #1155: FINERACT-1079: Run spotlessApply before compileJava task
Posted by GitBox <gi...@apache.org>.
awasum commented on pull request #1155:
URL: https://github.com/apache/fineract/pull/1155#issuecomment-654024763
> @awasum @xurror @thesmallstar @vorburger Just one concern: doesn't this mean that you can then check in code that doesn't pass the Spotless formatting standards, and get that to pass the build anyway?
>
> In other words: if I now commit code that is completely wrongly formatted, when Travis runs the build, it does spotlessApply so the build passes - but the **fixed code is never committed into Github**: so if I look at the source code on Github (or clone/branch the repository), it is still incorrectly formatted?
>
> This to me would seem to defeat the whole purpose of Spotless: it should ensure that the code in Github is correctly formatted, not that the code that is compiled is formatted correctly. The compiler doesn't care about code formatting...
>
> Also this means that the code everyone is running is actually different from what is checked into Github. So if you e.g. look at a stack trace from someone's build and compare the line numbers with source in Github - then the line numbers will not match, as spotlessApply may have changed them. Also this means that any source code you write is automatically modified by spotless without you getting a chance to review whether the changes are correct.
>
> Unless I'm missing something here, then this does not sound like a good idea to me...
@ptuomola ..I now see this as a problem than a solution. Thank you very much for catching this. I did not see all the edge cases and frankly should have put this as draft. I am not reverting this for us to look for a better solution. We should be looking for something like this: https://cwiki.apache.org/confluence/display/BEAM/Spotless+pre-commit
https://medium.com/@aruny3/improve-code-formatting-on-every-commit-7fbb0cdfdab6
https://github.com/diffplug/spotless/issues/178
----------------------------------------------------------------
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 pull request #1155: FINERACT-1079: Run spotlessApply before compileJava task
Posted by GitBox <gi...@apache.org>.
xurror commented on pull request #1155:
URL: https://github.com/apache/fineract/pull/1155#issuecomment-653990216
Yes. I've been thinking about this the very first time they asked me to run spotlessApply. It was kind of a PITA. I have no objections Concerning this. Thanks sending 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] thesmallstar commented on pull request #1155: FINERACT-1079: Run spotlessApply before compileJava task
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #1155:
URL: https://github.com/apache/fineract/pull/1155#issuecomment-653942926
I think if you do this spotlessCheck would run in the end.
But, SpotlessApply would have already fixed the violations (since it ran first).
I would suggest deliberately do a spotless Violation and check if it does actually fix it, if it does I think we can safely 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 pull request #1155: FINERACT-1079: Run spotlessApply before compileJava task
Posted by GitBox <gi...@apache.org>.
awasum commented on pull request #1155:
URL: https://github.com/apache/fineract/pull/1155#issuecomment-653942591
Is this a good idea? @thesmallstar , @xurror , @ptuomola ?
----------------------------------------------------------------
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