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: [email protected]
