[GitHub] [fineract] vorburger commented on a change in pull request #1290: Added and Enforced AbbreviationAsWordInName Checkstyle (FINERACT-821)

[GitBox <gi...@apache.org>]

vorburger commented on a change in pull request #1290:
URL: https://github.com/apache/fineract/pull/1290#discussion_r492973111



##########
File path: fineract-provider/src/main/java/org/apache/fineract/useradministration/api/RolesApiResourceSwagger.java
##########
@@ -24,7 +24,7 @@
 /**
  * Created by sanyam on 23/8/17.
  */
-@SuppressWarnings({ "MemberName" })
+@SuppressWarnings({ "MemberName", "AbbreviationAsWordInName" })

Review comment:
       here it's probably OK, because you can't change it, as it's already part of the external Swagger API? But then it's worth to document it like this:
   
   ```suggestion
   // Cannot change names of public API without breaking backwards compatibility
   @SuppressWarnings({ "MemberName", "AbbreviationAsWordInName" })
   ```

##########
File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/ClientSavingsIntegrationTest.java
##########
@@ -60,7 +60,7 @@
 /**
  * Client Savings Integration Test for checking Savings Application.
  */
-@SuppressWarnings({ "rawtypes" })
+@SuppressWarnings({ "rawtypes", "AbbreviationAsWordInName" })

Review comment:
       are there many problems in this test, would it be hard to also fix this, just so that it's nice?

##########
File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/ClientLoanIntegrationTest.java
##########
@@ -65,7 +65,7 @@
  * Client Loan Integration Test for checking Loan Application Repayments Schedule, loan charges, penalties, loan
  * repayments and verifying accounting transactions
  */
-@SuppressWarnings({ "rawtypes", "unchecked" })
+@SuppressWarnings({ "rawtypes", "unchecked", "AbbreviationAsWordInName" })
 public class ClientLoanIntegrationTest {

Review comment:
       @thesmallstar I would actually have to agree with @ptuomola that if just a little bit more work, it would be nice to finish this up.. but it depends on the effort, of course. How about we agree that we don't use `@SuppressWarnings("AbbreviationAsWordInName")` at least in the 2-3 `main` (non-`test`) code places I pointed out in this review, but that if there is any `test` code where it's an unreasonable amount of work to change it, we merge it anyway - would that seem fair?

##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/configuration/service/ExternalServicesConstants.java
##########
@@ -21,6 +21,7 @@
 import java.util.HashSet;
 import java.util.Set;
 
+@SuppressWarnings({ "AbbreviationAsWordInName" })

Review comment:
       it would be nice to fix this as well.. I'm hoping Checkstyle is OK with `S3_SERVICE_NAME` (because those truly are global constants), but it probably just doesn't like `ExternalservicePropertiesJSONinputParams` and the others like it? Again, just refactor those to be e.g. `ExternalservicePropertiesJsonInputParams`, that's better, and more consistent.

##########
File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/GroupSavingsIntegrationTest.java
##########
@@ -53,7 +53,7 @@
 /**
  * Group Savings Integration Test for checking Savings Application.
  */
-@SuppressWarnings({ "rawtypes", "unused" })
+@SuppressWarnings({ "rawtypes", "unused", "AbbreviationAsWordInName" })

Review comment:
       as above, would it be hard to fix this also here?

##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/configuration/domain/ConfigurationDomainService.java
##########
@@ -21,6 +21,7 @@
 import java.util.Date;
 import org.apache.fineract.infrastructure.cache.domain.CacheType;
 
+@SuppressWarnings({ "AbbreviationAsWordInName" })

Review comment:
       this is not test code, so it should be fixed.. is it because of `isSMSOTPDeliveryEnabled` etc.? Just make that e.g. `isSmsOtpDeliveryEnabled` - it's fine, and actually more not less readable (once you are used to the "convention" - and it's better to have it "uniform" across the entire code base).




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