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/06 20:28:42 UTC
[GitHub] [fineract] percyashu opened a new pull request #989: FINERACT-822 enable MissingCasesInEnumSwitch error
percyashu opened a new pull request #989:
URL: https://github.com/apache/fineract/pull/989
FINERACT-822
----------------------------------------------------------------
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 #989: FINERACT-822 enable MissingCasesInEnumSwitch error
Posted by GitBox <gi...@apache.org>.
awasum commented on pull request #989:
URL: https://github.com/apache/fineract/pull/989#issuecomment-640243529
@vorburger , @ptuomola , Both your points make sense to me looking back at this. @percyashu , Please, try to investigate further and take account this feedback. I will also look into this. Anyone is free to fix this error.
----------------------------------------------------------------
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 #989: FINERACT-822 enable MissingCasesInEnumSwitch error
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #989:
URL: https://github.com/apache/fineract/pull/989#issuecomment-640215857
@percyashu @awasum I'm not sure I agree with this... I haven't looked very closely, but what Error Prone seems to raise here is that the code seems to forget to handle certain cases of enum values, right?
For example, in `AccountNumberFormatDataValidator`'s `determineValidAccountNumberPrefixes()` it does not handle an `EntityAccountType` of type `SHARES`. By just putting `default:` everywhere, we're just... "shutting it up" - agreed? Is that really right? I don't understand enough of the functionality, but it seems to me that code is missing handling of certain... new configurations, or something.
I believe a better fix may be to add e.g. `case SHARES:` (etc.) and then... "do the appropriate thing". Or, perhaps at least `LOG.error("TODO Implement ValidAccountNumberPrefixes for SHARES")` etc. kind of thing?
@thesmallstar @ptuomola thought this code quality related discussion may interest you - feel free to chime in. (Manthan this may be related to a similar Enum switch Checkstyle validation could have, I can't remember; you could work together with @percyashu on this, if you're interested.)
----------------------------------------------------------------
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 #989: FINERACT-822 enable MissingCasesInEnumSwitch error
Posted by GitBox <gi...@apache.org>.
percyashu commented on pull request #989:
URL: https://github.com/apache/fineract/pull/989#issuecomment-640276604
Ok will do 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 pull request #989: FINERACT-822 enable MissingCasesInEnumSwitch error
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #989:
URL: https://github.com/apache/fineract/pull/989#issuecomment-640242074
I would have the same concern as @vorburger: my understanding was that the warning was correctly stating that some cases are not handled. Adding a default case will of course make the warning shut up, but do we know if a "no action" default results in the right handling for all the missing cases?
Even if "do nothing" is the right behaviour for the missing cases, I'd still recommend adding those cases explicitly to the switch statement. That way if someone adds more values for the enum, they are forced to consider what would be the right behaviour for the new values...
----------------------------------------------------------------
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 #989: FINERACT-822 enable MissingCasesInEnumSwitch error
Posted by GitBox <gi...@apache.org>.
awasum merged pull request #989:
URL: 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] thesmallstar commented on pull request #989: FINERACT-822 enable MissingCasesInEnumSwitch error
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #989:
URL: https://github.com/apache/fineract/pull/989#issuecomment-640277772
Seems I am little late here 😸
I was banging my head a few days back on the similar missing switch default check style(as @vorburger pointed out), looks like with this and #997 we have solved this?
We can look it again when we add that check style ;)
----------------------------------------------------------------
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