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/06/08 02:52:20 UTC
[GitHub] [fineract] percyashu opened a new pull request #1006: FINERACT-822 add MissingCasesInEnumSwitch
percyashu opened a new pull request #1006:
URL: https://github.com/apache/fineract/pull/1006
Base on changes requested in https://github.com/apache/fineract/pull/989
----------------------------------------------------------------
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 #1006: FINERACT-822 add MissingCasesInEnumSwitch
Posted by GitBox <gi...@apache.org>.
awasum commented on pull request #1006:
URL: https://github.com/apache/fineract/pull/1006#issuecomment-658189814
I am thinking of just merging this and seing what happens. @percyashu , manually rebase this up top of latest develop and lets get this in. Worst case, we reverse this or someone who uses the whole term feature sends a PR for it. Other choice is to not merge this and allow someone to fully implement this by picking up this: https://issues.apache.org/jira/browse/FINERACT-1080
----------------------------------------------------------------
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 #1006: FINERACT-822 add MissingCasesInEnumSwitch
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #1006:
URL: https://github.com/apache/fineract/pull/1006#issuecomment-660982627
@percyashu @ptuomola
Do you think we stil need/there is any benefit for: MissingSwitchDefault Checktyle?
MissingSwitchDefault -> Forces to have default statement on every switch. Are we not doing a better job here already?
----------------------------------------------------------------
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 #1006: FINERACT-822 add MissingCasesInEnumSwitch
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #1006:
URL: https://github.com/apache/fineract/pull/1006#issuecomment-651341203
@percyashu where are we with this? It would probably be good to wrap this one up and get it merged soon-ish now?
@ptuomola does this one have LGTM from you 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] awasum merged pull request #1006: FINERACT-822 add MissingCasesInEnumSwitch
Posted by GitBox <gi...@apache.org>.
awasum merged pull request #1006:
URL: https://github.com/apache/fineract/pull/1006
----------------------------------------------------------------
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 #1006: FINERACT-822 add MissingCasesInEnumSwitch
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #1006:
URL: https://github.com/apache/fineract/pull/1006#issuecomment-644290570
@ptuomola looks like@percyashu would like this to be re-reviewed.
----------------------------------------------------------------
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] percyashu commented on pull request #1006: FINERACT-822 add MissingCasesInEnumSwitch
Posted by GitBox <gi...@apache.org>.
percyashu commented on pull request #1006:
URL: https://github.com/apache/fineract/pull/1006#issuecomment-642955506
thanks @ptuomola for the feedback. I will address the requested changes
----------------------------------------------------------------
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 #1006: FINERACT-822 add MissingCasesInEnumSwitch
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #1006:
URL: https://github.com/apache/fineract/pull/1006#issuecomment-641592588
@vorburger @ptuomola @percyashu
I was researching a bit on this after reading your comments, I found a few reasons why we should always have a default case, I am quoting from checkstyle docs.
Rationale: It's usually a good idea to introduce a default case in every switch statement. Even if the developer is sure that all currently possible cases are covered, this should be expressed in the default branch, e.g. by using an assertion. This way the code is protected against later changes, e.g. introduction of new types in an enumeration type.
----------------------------------------------------------------
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] percyashu commented on pull request #1006: FINERACT-822 add MissingCasesInEnumSwitch
Posted by GitBox <gi...@apache.org>.
percyashu commented on pull request #1006:
URL: https://github.com/apache/fineract/pull/1006#issuecomment-653594619
@awasum I will fix conflict now and concerning WHOLE_TERM case as @ptuomola point out
> Is there someone with more functional knowledge on this particular logic that could comment on the WHOLE_TERM case?
And if WHOLE_TERM case is missing and is needed to be implemented, I will like to work on it with pointers on the logic to use.
----------------------------------------------------------------
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 #1006: FINERACT-822 add MissingCasesInEnumSwitch
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1006:
URL: https://github.com/apache/fineract/pull/1006#issuecomment-652771736
@vorburger @percyashu I don't think this makes any changes to the functionality so in that way should be safe to merge.
The bit I'm not sure about is the logging of error for WHOLE_TERM cases. Is this really missing logic, or is it that the default case does the right thing for WHOLE_TERM anyway? I don't know what is the expected behaviour of these functions for the WHOLE_TERM case (or if they even get called with WHOLE_TERM as argument) - and none of our tests seem to hit this case.
Is there someone with more functional knowledge on this particular logic that could comment on the WHOLE_TERM case?
I suppose in the worst case we commit this and consequently get some additional / misleading error messages in the log. Not the end of the world - but of course not great either, given that this PR is supposed to make the code more maintainable...
----------------------------------------------------------------
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] percyashu commented on a change in pull request #1006: FINERACT-822 add MissingCasesInEnumSwitch
Posted by GitBox <gi...@apache.org>.
percyashu commented on a change in pull request #1006:
URL: https://github.com/apache/fineract/pull/1006#discussion_r440320850
##########
File path: fineract-provider/src/main/java/org/apache/fineract/organisation/workingdays/domain/WorkingDaysEnumerations.java
##########
@@ -53,7 +53,10 @@ public static EnumOptionData repaymentRescheduleType(final RepaymentRescheduleTy
optionData = new EnumOptionData(RepaymentRescheduleType.MOVE_TO_PREVIOUS_WORKING_DAY.getValue().longValue(),RepaymentRescheduleType.MOVE_TO_PREVIOUS_WORKING_DAY.getCode(),
"move to previous working day");
break;
- default:
+ case MOVE_TO_NEXT_MEETING_DAY:
Review comment:
Yeah it doesn't seems to be used anywhere. It was added by @nazeer1100126 and he might have a plan for its addition.
----------------------------------------------------------------
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 #1006: FINERACT-822 add MissingCasesInEnumSwitch
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #1006:
URL: https://github.com/apache/fineract/pull/1006#discussion_r444411453
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/Loan.java
##########
@@ -4501,7 +4505,9 @@ private LocalDate getMaxDateLimitForNewRepayment(final PeriodFrequencyType perio
break;
case INVALID:
break;
- default:
Review comment:
But then should we be logging error in other places below, or do like here? isn't it inconsistent how is 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] percyashu commented on pull request #1006: FINERACT-822 add MissingCasesInEnumSwitch
Posted by GitBox <gi...@apache.org>.
percyashu commented on pull request #1006:
URL: https://github.com/apache/fineract/pull/1006#issuecomment-643756592
@vorburger not fully yet. Will request a review from @ptuomola when complete
----------------------------------------------------------------
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 a change in pull request #1006: FINERACT-822 add MissingCasesInEnumSwitch
Posted by GitBox <gi...@apache.org>.
ptuomola commented on a change in pull request #1006:
URL: https://github.com/apache/fineract/pull/1006#discussion_r437839268
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/domain/SavingsAccountCharge.java
##########
@@ -292,7 +296,9 @@ private void populateDerivedFields(final BigDecimal transactionAmount, final Big
this.amountWaived = null;
this.amountWrittenOff = null;
break;
- default:
+ case PERCENT_OF_DISBURSEMENT_AMOUNT:
Review comment:
As far as I can see, this case should never happen: we should not have a savings account with a charge type of PERCENT_OF_DISBURSEMENT_AMOUNT, as that just doesn't make sense (and is not even valid as set in validValuesForSavings()). So the behaviour for this case in these functions should be the same as for the other values that are not valid - e.g. PERCENT_OF_AMOUNT_AND_INTEREST
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/accountnumberformat/data/AccountNumberFormatDataValidator.java
##########
@@ -127,7 +130,9 @@ public void validateForCreate(final String json) {
case GROUP:
validAccountNumberPrefixes = AccountNumberFormatEnumerations.accountNumberPrefixesForGroups;
break;
- default:
+ case SHARES:
+ LOG.error("TODO Implement determineValidAccountNumberPrefixes for SHARES");
+ break;
}
Review comment:
If you look in AccountNumberGenerator.generateAccountNumber(), it seems that we currently are not generating any prefix for SHARES type accounts. Similarly AccountNumberFormatEnumerations does not provide a prefix for SHARES accounts. So means there are currently no valid prefixes for SHARES accounts - and therefore returning an empty list (which was also existing behaviour) would seem to be the correct behaviour.
Therefore to me this is therefore not a "TODO": returning the empty list is correct as the functionality stands. Of course we could implement account prefixes for SHARES accounts but that would not require just fixing this code, but all the other places as well...
##########
File path: fineract-provider/src/main/java/org/apache/fineract/organisation/workingdays/domain/WorkingDaysEnumerations.java
##########
@@ -53,7 +53,10 @@ public static EnumOptionData repaymentRescheduleType(final RepaymentRescheduleTy
optionData = new EnumOptionData(RepaymentRescheduleType.MOVE_TO_PREVIOUS_WORKING_DAY.getValue().longValue(),RepaymentRescheduleType.MOVE_TO_PREVIOUS_WORKING_DAY.getCode(),
"move to previous working day");
break;
- default:
+ case MOVE_TO_NEXT_MEETING_DAY:
Review comment:
I wonder if this enum value is actually used for anything? I can't find any reference to it in the Fineract code. The code that processes all the other repaymentRescheduleTypeOptions (e.g. WorkingDaysUtil.getOffSetDateIfNonWorkingDay()) does not have a case or any logic for this value.
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/Loan.java
##########
@@ -4501,7 +4505,9 @@ private LocalDate getMaxDateLimitForNewRepayment(final PeriodFrequencyType perio
break;
case INVALID:
break;
- default:
Review comment:
Looks like the whole repayment / interest rate calculation etc logic for WHOLE_TERM is missing in a number of functions.
What we would need to work out is: is this intentional (i.e. the routines, without any handling for WHOLE_TERM, will still return the right value) or is this actually broken even now? I'd suggest trying this out e.g. through the community app to see if loan products with whole term interest actually work correctly in scenarios where these functions are called.
If they do, then the right thing to do would be to add a case that does nothing for WHOLE_TERM so we have made this behaviour explicit. If they don't work, then we should have an error message with a TO DO.
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/note/api/NotesApiResource.java
##########
@@ -228,7 +232,12 @@ private CommandWrapper getResourceDetails(final NoteType type, final Long resour
resourceNameForPermissions = "GROUPNOTE";
resourceDetails.withGroupId(resourceId);
break;
- default:
+ case SHARE_ACCOUNT:
Review comment:
If you look in createNote(), you can see that we only create specific types of notes - i.e. a subset of the enum types available. So we should never get a note with one of these types. In my view, therefore I think it would be OK to have a default that returns "INVALIDTYPE" for any other type.
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/accountnumberformat/service/AccountNumberFormatReadPlatformServiceImpl.java
##########
@@ -145,7 +149,9 @@ public void determinePrefixTypesForAccounts(Map<String, List<EnumOptionData>> ac
case GROUP :
accountNumberPrefixTypesSet = AccountNumberFormatEnumerations.accountNumberPrefixesForGroups;
break;
- default:
+ case SHARES:
+ LOG.error("TODO Implement determinePrefixTypesForAccounts for SHARES");
+ break;
Review comment:
Same here as above - I don't think we need to log an error, I would just have a comment stating no prefixes for SHARES accounts:
case SHARES:
// SHARES accounts have no prefix
break;
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/interestratechart/service/InterestRateChartEnumerations.java
##########
@@ -52,7 +56,9 @@ public static EnumOptionData periodType(final PeriodFrequencyType type) {
optionData = new EnumOptionData(PeriodFrequencyType.YEARS.getValue().longValue(),
PeriodFrequencyType.YEARS.getCode(), "Years");
break;
- default:
+ case WHOLE_TERM:
+ LOG.error("TODO Implement repaymentPeriodFrequencyType for WHOLE_TERM");
+ break;
Review comment:
Is there a reason why we can't just return the optionData for WHOLE_TERM just like for all the other values?
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/shareaccounts/serialization/ShareAccountDataSerializer.java
##########
@@ -927,7 +931,9 @@ private LocalDate deriveLockinPeriodDuration(final Integer lockinPeriod, final P
case YEARS:
lockinDate = purchaseDate.plusYears(lockinPeriod) ;
break ;
- default:
+ case WHOLE_TERM:
Review comment:
I don't think a lockinPeriod of WHOLE_TERM makes sense functionally - so shouldn't the behaviour for this to be the same as for period INVALID?
----------------------------------------------------------------
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 #1006: FINERACT-822 add MissingCasesInEnumSwitch
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1006:
URL: https://github.com/apache/fineract/pull/1006#issuecomment-641687223
@thesmallstar well... that's definitely true, but only IF there is a default case that we know will make sense for new cases that may be added AND / OR if we don't have a mechanism to alert the developer if they have forgotten to add a case for a new type they've introduced.
But in this case, thanks to ErrorProne, we can do better than with just standard Java: we have a choice of not needing to worry about whether the default case will work for any new cases, because ErrorProne will warn us if we add a new case that isn't handled
So my view would be:
- If you can write a default case that is really "default" - i.e. the common way of handling all cases where no specific handling is needed - with all specific cases being "exceptions" from the default, then by all means let's have a default case
- If there is no clear default - i.e. the switch chooses from different types of behaviour, with no clear way of saying what is the common way of doing things - then in my view the cases should be explicitly named. In this way, when some adds a new value for the enum, they are also forced by Error Prone to think about the handling for that case.
In this PR, there are both types of cases:
- We can argue that the default for account number prefix is that the account has no prefix, with specific cases then saying that specific types of accounts can have prefixes. So in this case a default of returning an empty list of prefixes makes sense.
- But it doesn't make sense to that e.g. the default interest term is one month. Each interest term is different (one week, one month, one year) and there is no "default" we can assume will lead to a right behaviour. If we say that we use e.g. one month as default for everything we don't recognise, we will get wrong results for every new term that may be added. So in such case it makes sense to force the developer who adds a term to also add a new case in the routines that need to work on that term.
Just my thoughts - happy to discuss...
----------------------------------------------------------------
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 #1006: FINERACT-822 add MissingCasesInEnumSwitch
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #1006:
URL: https://github.com/apache/fineract/pull/1006#issuecomment-640369218
@ptuomola would you perhaps like to help code review this PR?
----------------------------------------------------------------
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 #1006: FINERACT-822 add MissingCasesInEnumSwitch
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #1006:
URL: https://github.com/apache/fineract/pull/1006#issuecomment-657814814
@awasum what do you want to do re. 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] vorburger commented on pull request #1006: FINERACT-822 add MissingCasesInEnumSwitch
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #1006:
URL: https://github.com/apache/fineract/pull/1006#issuecomment-647042625
@ptuomola does this one have LGTM from you 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] percyashu commented on pull request #1006: FINERACT-822 add MissingCasesInEnumSwitch
Posted by GitBox <gi...@apache.org>.
percyashu commented on pull request #1006:
URL: https://github.com/apache/fineract/pull/1006#issuecomment-654591802
Created an issue for the WHOLE_TERM case [this](https://issues.apache.org/jira/browse/FINERACT-1080).
@awasum @ptuomola should this Pr be closed or merged while awaiting someone with more functional knowledge about this case to chime in?
----------------------------------------------------------------
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] percyashu commented on pull request #1006: FINERACT-822 add MissingCasesInEnumSwitch
Posted by GitBox <gi...@apache.org>.
percyashu commented on pull request #1006:
URL: https://github.com/apache/fineract/pull/1006#issuecomment-651801774
@vorburger I have address all requested changes and am waiting for another review @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
[GitHub] [fineract] thesmallstar commented on pull request #1006: FINERACT-822 add MissingCasesInEnumSwitch
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #1006:
URL: https://github.com/apache/fineract/pull/1006#issuecomment-646958824
@percyashu @ptuomola I was trying to add a checkstyle, that makes it necessary to use default in switch. From the discussion above, error-prone handles this in a much more intelligent way. Do you think the same? If yes, @vorburger will it be a better idea to remove that checkstyle and mention in comment about how we are handling missing defaults.
----------------------------------------------------------------
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] percyashu commented on pull request #1006: FINERACT-822 add MissingCasesInEnumSwitch
Posted by GitBox <gi...@apache.org>.
percyashu commented on pull request #1006:
URL: https://github.com/apache/fineract/pull/1006#issuecomment-645214196
@ptuomola 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] percyashu commented on pull request #1006: FINERACT-822 add MissingCasesInEnumSwitch
Posted by GitBox <gi...@apache.org>.
percyashu commented on pull request #1006:
URL: https://github.com/apache/fineract/pull/1006#issuecomment-661250752
I think we are doing a better job @thesmallstar
----------------------------------------------------------------
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] percyashu commented on a change in pull request #1006: FINERACT-822 add MissingCasesInEnumSwitch
Posted by GitBox <gi...@apache.org>.
percyashu commented on a change in pull request #1006:
URL: https://github.com/apache/fineract/pull/1006#discussion_r440341512
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/Loan.java
##########
@@ -4501,7 +4505,9 @@ private LocalDate getMaxDateLimitForNewRepayment(final PeriodFrequencyType perio
break;
case INVALID:
break;
- default:
Review comment:
I look into this and Loan products with whole term seem to 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
[GitHub] [fineract] vorburger commented on pull request #1006: FINERACT-822 add MissingCasesInEnumSwitch
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #1006:
URL: https://github.com/apache/fineract/pull/1006#issuecomment-643756337
@percyashu I'm unclear if all of @ptuomola review feedback has already been addressed here, or not yet fully?
@ptuomola I'll let you lead the review of this PR, and just merge it when it has your LGTM.
PS: This may get in conflict with #943 later today.
----------------------------------------------------------------
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] percyashu commented on pull request #1006: FINERACT-822 add MissingCasesInEnumSwitch
Posted by GitBox <gi...@apache.org>.
percyashu commented on pull request #1006:
URL: https://github.com/apache/fineract/pull/1006#issuecomment-658309025
OK @awasum I have done that
----------------------------------------------------------------
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 a change in pull request #1006: FINERACT-822 add MissingCasesInEnumSwitch
Posted by GitBox <gi...@apache.org>.
ptuomola commented on a change in pull request #1006:
URL: https://github.com/apache/fineract/pull/1006#discussion_r444616087
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/Loan.java
##########
@@ -4501,7 +4505,9 @@ private LocalDate getMaxDateLimitForNewRepayment(final PeriodFrequencyType perio
break;
case INVALID:
break;
- default:
Review comment:
As far as I can see, there are three options for this WHOLE_TERM logic (and other "missing cases"):
1. These functions are not needed when enum is set to WHOLE_TERM and are never called with that value (e.g. there is no need to determine max date for repayment when you are paying for the whole term in one go). In that case the logic for WHOLE_TERM is not needed in the functions and we should not log an error as there is nothing to be done.
2. These functions are needed when enum is set to WHOLE_TERM and are called with that value. However the default logic - i.e. do nothing in the switch - results in the function returning the right value. If that's the case, then we should just make this explicit: add a case for WHOLE_TERM that does nothing, and a comment that this has been added for a reason. But no need to log an error as the "do nothing" behaviour is correct.
3. These functions are needed when enum is set to WHOLE_TERM but the logic to handle WHOLE_TERM has not been implemented. If that's the case, the function is currently returning an incorrect value when called with WHOLE_TERM. We should then add the case and a log statement to output a TODO to indicate that the logic is missing.
From response of @percyashu to my earlier question, I understood that in this case the answer is either #1 or #2 - so logging nothing is correct. But in other switch statements in the code, the answer may be #3 so logging an error / TODO would be correct.
In my view, to see if this PR is correct, we'd need to categorise each of the switch statements to one of the above cases. I started doing that and went through them once earlier, but there are a couple of cases (like this WHOLE_TERM) where it's difficult to be sure just by looking at the code...
----------------------------------------------------------------
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 #1006: FINERACT-822 add MissingCasesInEnumSwitch
Posted by GitBox <gi...@apache.org>.
awasum commented on pull request #1006:
URL: https://github.com/apache/fineract/pull/1006#issuecomment-653529204
> @vorburger @percyashu I don't think this makes any changes to the functionality so in that way should be safe to merge.
>
> The bit I'm not sure about is the logging of error for WHOLE_TERM cases. Is this really missing logic, or is it that the default case does the right thing for WHOLE_TERM anyway? I don't know what is the expected behaviour of these functions for the WHOLE_TERM case (or if they even get called with WHOLE_TERM as argument) - and none of our tests seem to hit this case.
>
> Is there someone with more functional knowledge on this particular logic that could comment on the WHOLE_TERM case?
>
> I suppose in the worst case we commit this and consequently get some additional / misleading error messages in the log. Not the end of the world - but of course not great either, given that this PR is supposed to make the code more maintainable...
@percyashu please fix the merge conflicts. Do you want to also try to implement the WHOLE_TERM case?
----------------------------------------------------------------
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 a change in pull request #1006: FINERACT-822 add MissingCasesInEnumSwitch
Posted by GitBox <gi...@apache.org>.
awasum commented on a change in pull request #1006:
URL: https://github.com/apache/fineract/pull/1006#discussion_r444762583
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/loanschedule/domain/DefaultPaymentPeriodsInOneYearCalculator.java
##########
@@ -45,7 +49,9 @@ public Integer calculate(final PeriodFrequencyType repaymentFrequencyType) {
case INVALID:
paymentPeriodsInOneYear = Integer.valueOf(0);
break;
- default:
+ case WHOLE_TERM:
+ LOG.error("TODO Implement repaymentFrequencyType for WHOLE_TERM");
Review comment:
Can this whole term case be implemented? Or is it too complicated? Can we have an issue opened for it? How will the whole term case even 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