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/26 12:46:02 UTC
[GitHub] [fineract] thesmallstar opened a new pull request #985: FINERACT-821 Enforced Membername checkstyle(1)
thesmallstar opened a new pull request #985:
URL: https://github.com/apache/fineract/pull/985
This is more for discussion currently:
So checkstyle believe a constant is a variable that is both "static" and "final" definitely true, but it a little big arguable do we really also need a static keyword to call it constant?
MemberName check style would want something like:
final int TOTAL_SUM to be changed to final int totalSum
That is name final variables not like constantNames.
Fineract extensively uses TOTAL_SUM way of naming things for final variables.
What should we follow?
@vorburger @awasum @xurror
----------------------------------------------------------------
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 #985: [WIP] MemberName Checkstyle
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #985:
URL: https://github.com/apache/fineract/pull/985#issuecomment-640209687
> final int TOTAL_SUM to be changed to final int totalSum
or you can make it `final static int TOTAL_SUM`, right? Then it's happy - and that's better.
FYI in Java, as far as I know, always making all constants `final static` is much better than only `final`, because this saves memory... a `final static` is defined only ONCE in the Class, but a `final` is stored in every INSTANCE of said Class (I think, at least that's probably how it was originally; perhaps nowadays that gets optimized?). For a Test or Service class this doesn't matter (as there is typically only 1 instance of `YourService` or `SomethingTest`), but for a "domain" class of which there are N in production at run-time, this could make a real difference. For consistency, the rule of always making all constants `static final` seems sensible, to me - and is a very widely used pseudo standard convention in Java anyway.
----------------------------------------------------------------
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 #985: FINERACT-821 Enforced Membername checkstyle(1)
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #985:
URL: https://github.com/apache/fineract/pull/985#issuecomment-663983477
failed due to connection error, closing and opening again.
----------------------------------------------------------------
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 #985: [WIP] MemberName Checkstyle
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #985:
URL: https://github.com/apache/fineract/pull/985#issuecomment-657814507
@thesmallstar will you update this before it gets auto closed because of stale?
----------------------------------------------------------------
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] github-actions[bot] commented on pull request #985: [WIP] MemberName Checkstyle
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #985:
URL: https://github.com/apache/fineract/pull/985#issuecomment-655207967
This pull request seems to be stale. Are you still planning to work on it? We will automatically close it in 30 days.
----------------------------------------------------------------
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 #985: FINERACT-821 Enforced Membername checkstyle(1)
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #985:
URL: https://github.com/apache/fineract/pull/985#discussion_r460723891
##########
File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/TemplateIntegrationTest.java
##########
@@ -34,9 +34,9 @@
public class TemplateIntegrationTest {
- private final String GET_TEMPLATES_URL = "/fineract-provider/api/v1/templates?tenantIdentifier=default";
- private final String GET_TEMPLATE_ID_URL = "/fineract-provider/api/v1/templates/%s?tenantIdentifier=default";
- private final String RESPONSE_ATTRIBUTE_NAME = "name";
+ private static final String GET_TEMPLATES_URL = "/fineract-provider/api/v1/templates?tenantIdentifier=default";
+
Review comment:
Yeah, only used once, and with the parameter, I would agree that the `String.format()` is EASIER to read, 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
[GitHub] [fineract] thesmallstar commented on pull request #985: FINERACT-821 Enforced Membername checkstyle(1)
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #985:
URL: https://github.com/apache/fineract/pull/985#issuecomment-663994620
@xurror @vorburger 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] thesmallstar commented on pull request #985: [WIP] MemberName Checkstyle
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #985:
URL: https://github.com/apache/fineract/pull/985#issuecomment-657815137
@vorburger 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] vorburger merged pull request #985: FINERACT-821 Enforced Membername checkstyle(1)
Posted by GitBox <gi...@apache.org>.
vorburger merged pull request #985:
URL: https://github.com/apache/fineract/pull/985
----------------------------------------------------------------
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 #985: FINERACT-821 Enforced Membername checkstyle(1)
Posted by GitBox <gi...@apache.org>.
xurror commented on a change in pull request #985:
URL: https://github.com/apache/fineract/pull/985#discussion_r460539551
##########
File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/TemplateIntegrationTest.java
##########
@@ -34,9 +34,9 @@
public class TemplateIntegrationTest {
- private final String GET_TEMPLATES_URL = "/fineract-provider/api/v1/templates?tenantIdentifier=default";
- private final String GET_TEMPLATE_ID_URL = "/fineract-provider/api/v1/templates/%s?tenantIdentifier=default";
- private final String RESPONSE_ATTRIBUTE_NAME = "name";
+ private static final String GET_TEMPLATES_URL = "/fineract-provider/api/v1/templates?tenantIdentifier=default";
+
Review comment:
Why remove this variable?
It was only used once though so I don't mind leaving it 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] thesmallstar closed pull request #985: FINERACT-821 Enforced Membername checkstyle(1)
Posted by GitBox <gi...@apache.org>.
thesmallstar closed pull request #985:
URL: https://github.com/apache/fineract/pull/985
----------------------------------------------------------------
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 #985: [WIP] MemberName Checkstyle
Posted by GitBox <gi...@apache.org>.
awasum commented on pull request #985:
URL: https://github.com/apache/fineract/pull/985#issuecomment-659527171
Is this still important? and also merge conflicts. @thesmallstar , If you have too many issues on your plate, please take one at a time and finish them except they are too complicated?
----------------------------------------------------------------
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 #985: FINERACT-821 Enforced Membername checkstyle(1)
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #985:
URL: https://github.com/apache/fineract/pull/985#issuecomment-664597865
Yes, this PR does not add any new checkstyle but enforces a part of member name checkstyle.
In this PR I have updated to support for integrationTests.
----------------------------------------------------------------
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 #985: [WIP] MemberName Checkstyle
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #985:
URL: https://github.com/apache/fineract/pull/985#issuecomment-659527873
Yes, I am trying to focus on newer issues. However this is important, I will restart the checkstyle work next week and wrap 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