fineract-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [fineract] awasum commented on pull request #1006: FINERACT-822 add MissingCasesInEnumSwitch
Date Fri, 03 Jul 2020 12:40:45 GMT

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



Mime
View raw message