From commits-return-10868-archive-asf-public=cust-asf.ponee.io@fineract.apache.org Fri Jul 3 12:40:46 2020 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [207.244.88.153]) by mx-eu-01.ponee.io (Postfix) with SMTP id 2116618057A for ; Fri, 3 Jul 2020 14:40:46 +0200 (CEST) Received: (qmail 61459 invoked by uid 500); 3 Jul 2020 12:40:45 -0000 Mailing-List: contact commits-help@fineract.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@fineract.apache.org Delivered-To: mailing list commits@fineract.apache.org Received: (qmail 61450 invoked by uid 99); 3 Jul 2020 12:40:45 -0000 Received: from ec2-52-202-80-70.compute-1.amazonaws.com (HELO gitbox.apache.org) (52.202.80.70) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 03 Jul 2020 12:40:45 +0000 From: =?utf-8?q?GitBox?= To: commits@fineract.apache.org Subject: =?utf-8?q?=5BGitHub=5D_=5Bfineract=5D_awasum_commented_on_pull_request_=2310?= =?utf-8?q?06=3A_FINERACT-822_add_MissingCasesInEnumSwitch?= Message-ID: <159378004540.29655.3030675663368706830.asfpy@gitbox.apache.org> Date: Fri, 03 Jul 2020 12:40:45 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit In-Reply-To: References: 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