fineract-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [fineract] vorburger edited a comment on pull request #943: FINERACT-1006 Added spotless to auto format source code
Date Mon, 01 Jun 2020 10:09:05 GMT

vorburger edited a comment on pull request #943:
URL: https://github.com/apache/fineract/pull/943#issuecomment-636757443


   @thesmallstar So this idea look very interesting, to me; thanks for having investigated
this! But I'd like to fully understand exactly what we are doing here a little more myself,
before merging it... please know that, even though I've originally suggested you look into
Spotless in FINERACT-1006, I've actually not read much about it myself... :smile: so I'm looking
for your guidance to fully learn about it myself.
   
   In order to make a full review and merging more easy and avoid conflicts, could I make
a suggestion, just from a "practical" point of view? How about in this PR, you revert the
.java code changes and ONLY propose whatever we need to add to Gradle (and README, hopefully?)
and config files, but do not actually include the result of having run it to change the Java
files? That makes it easier for me to completely review it properly, and hopefully eventually
merge. We could then, in a separate next step, actually run and merge the result (or I could
just do that myself, and merge it "quickly", to avoid delays).
   
   One thing I haven't completely understood yet here is if this "just" adds a new way to
Auto Format by launching a certain Gradle task (include doc in README...) or if this, like
Checkstyle and Error Prone & Co. will actually verify formatting, and fail the build?
Or is the idea that we leave that up to Checkstyle only, but that folks can then use this
to make changes comply? Or both? It's something worth clearly documenting in the README, IMHO.
   
   I guess what would be "ideal" is if this would both verify formatting, fail the build if
NOK (like Checkstyle; I wouldn't mind if this PR fails to build, because of that), but if
such failures would show a clear message à la "Hey buddy, you can easily fix your code's
ugly formatting by running `./gradlew something`.... 


----------------------------------------------------------------
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



Mime
View raw message