ptuomola commented on pull request #1828: URL: https://github.com/apache/fineract/pull/1828#issuecomment-898983266
Hi Danish - many thanks for all of your pull requests fixing the Swagger client API definitions - this is great work! What I was thinking about is: how do we best review these and ensure the fixes are correct. One option would be for me to simply look at the changes you’ve made and compare that to the function signature manually. That of course works, but is pretty error-prone and manual: if we make any mistakes in comparison, then there may still be differences and the Swagger client will not work. Also this is “one-off”: if there are any changes in the future to either Swagger definition or to the function signature, there is no guarantee they will stay in synch. To me, the real solution would be to actually write some test cases that use the generated Swagger client to call the APIs. If they work, then clearly the definitions are correct as well. If they don’t work, then there’s still some difference (or some other problem) with the Swagger definitions. The other benefit with this would be that these test cases could then be part of the automatic build, and ensure the APIs remain working also in the future. In fact, we’ve before discussed that it would make sense to move all the current integration tests (which work via direct REST calls to the API) to be done through the Swagger generated client instead. Would be great to get your thoughts - if there’s an easier way to make sure things are “in synch” then let’s consider that as well. Alternatively, if you think writing tests makes sense as an approach, would you have time to also write some simple test cases through the generated Swagger client for the fixes that you submit? That way their correctness would be automatically verified both now and in the future. Cc’ing the Dev list for any other ideas/suggestions as well Thanks Best regards Petri > On 15 Aug 2021, at 9:31 AM, Danish Jamal ***@***.***> wrote: > > Description > > Field renamed to match the api actual response, below are the fields renamed. > > appTableName to applicationTableName > datatableName to registeredTableName > column to columnHeaderData > For more info refer Apache Fineract JIRA ticket #1372 <https://issues.apache.org/jira/browse/FINERACT-1372>. > > Checklist > > Please make sure these boxes are checked before submitting your pull request - thanks! > > Write the commit message as per https://github.com/apache/fineract/#pull-requests <https://github.com/apache/fineract/#pull-requests> > Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers. > > Create/update unit or integration tests for verifying the changes made. > > Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions <https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions>. > > Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/api-docs/apiLive.htm with details of any API changes > > Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.) > > FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide <https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide>. > > You can view, comment on, or merge this pull request online at: > > https://github.com/apache/fineract/pull/1828 <https://github.com/apache/fineract/pull/1828> > Commit Summary > > invalid field name fixed in GetDataTablesResponse (FINERACT-1372) > File Changes > > M fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/api/DatatablesApiResourceSwagger.java <https://github.com/apache/fineract/pull/1828/files#diff-d584c3cc7b333d42e21267ba4362191970ad3edbff5e770285d634d690b0a708> (6) > Patch Links: > > https://github.com/apache/fineract/pull/1828.patch <https://github.com/apache/fineract/pull/1828.patch> > https://github.com/apache/fineract/pull/1828.diff <https://github.com/apache/fineract/pull/1828.diff> > — > You are receiving this because you are subscribed to this thread. > Reply to this email directly, view it on GitHub <https://github.com/apache/fineract/pull/1828>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AASJVCVR37FUEDXFRHUW7FTT44KHRANCNFSM5CFUS5QQ>. > Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>. > -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
