Hello everyone, I've made some changes to the approach for API documentation based on Myrle's feedback.
So I have reverted my recent changes to fineract-cn-customer and created a pull request [1] for review. I am hoping that we'll get all necessary fixes on the PR before I move on to documenting the other services. I have also documented the process [2] for API Documentation which stipulates how a developer can generate the snippets for documentation ( in component-test's build/doc/ folder ) given that we don't want .adoc file and such checked into the repository. I await your thoughts on this. [1] https://github.com/apache/fineract-cn-customer/pull/7 [2] https://cwiki.apache.org/confluence/display/FINERACT/Apache+Fineract+CN+API+Documentation On Wed, Apr 25, 2018 at 6:09 PM, Isaac Kamga <[email protected]> wrote: > Hello Myrle, > > I've done some tinkering and Spring REST Docs can indeed get the snippets > generated for documentation in the component-test module. At least, that > ensures that there's no need for an architectural change and the service > module doesn't depend on the component-test module as Markus advised. > > I think there are still other concerns which you highlighted; > > 1. Checking in generated documentation : Should we leave them in > component-test's build folder (which is ignored by git) and rather document > the process of generating the snippets ? > > 2. Creating a documentation module : Do we still need to try this out ? > This module will import the service and component-test modules. > > 3. MockMvc vs Spring Fox: Were you concerned about the security of MockMvc > ? > > I'd like to have this discussed here so that we don't proceed with > approaches to solving the problem which aren't worthwhile. I had earlier > assumed that a mention on the Quarter 1 board report about my work on API > documentation would have sufficed. > > If you're okay with the API documentation occurring in the component-test > module, it may be okay for me to revert the commits I made to the > repositories and submit a Pull request which you'll review and merge. > > At Your Service, > Isaac Kamga. > > On Wed, Apr 25, 2018 at 11:53 AM, Isaac Kamga <[email protected]> > wrote: > >> Hi Myrle, >> >> Thanks for your elaborate feedback and corrections. >> >> On Wed, Apr 25, 2018 at 9:10 AM, Myrle Krantz <[email protected]> wrote: >> >>> Hey Isaac, >>> >>> I've looked at your changes in customer and I've found several >>> problems with them: >>> >>> 1.) You've copied all the component tests out of the component test >>> module into the service module. This means that identical tests will >>> have to be maintained in two places. Because people aren't very good >>> at keeping code in sync, you can be certain that these tests will >>> diverge from each other a little more each time someone makes a >>> change, and contributors (myself included) won't know which tests are >>> authoritative. >> >> >>> 2.) You've removed the Apache license header from at least one file. >>> I added the Apache license header to the build files very recently, as >>> one step towards making the code releasable. We can't release the >>> code without the ASF license headers. >>> >> >> Ouch ! My bad. I would have left this commit as a PR for review first. >> >> >>> 3.) I'm assuming the documentation is generated? Do we really want to >>> check in generated documentation? And where is the process documented >>> for generating the documentation? >>> >> >> By default, the Spring REST Docs tool outputs documentation in the >> service's build folder ( which is ignored by git ), so I placed them in >> service/src/doc so they can be viewed. >> >> >>> >>> 4.) You added a compile dependency from service to test. The service >>> should not depend on test. It should be possible to make changes and >>> add environment variables and etc to the test repository without even >>> thinking about whether a change might break production code or turn on >>> test-configurations which weaken security in production code. >>> >>> In general dependencies between modules must be 1-way, or else >>> compilation is difficult or impossible. Component-tests should depend >>> on service. Service should not depend on component-test. Markus >>> mentioned this when you asked (2) You might ask, why are component >>> tests a separate module? There are several reasons: >>> >> >> I was concerned about duplicating the component-tests' tests in the >> service module and asked about how to avoid the duplication for which I had >> no feedback.(2) >> >> >>> * We may wish to release the service and api modules and not release >>> the component-test module. This opens up some possibilities for the >>> tests that would otherwise be closed off by ASF licensing >>> requirements. >>> * Keeping the code seperate helps contributors keep the concerns of >>> testing and the concern of functionality separate in their thought >>> processes. Putting testing code in services in other projects has >>> resulted in poor code or even major security vulnerabilities. It's >>> important here to think about solutions and their trade-offs carefully >>> rather than just slapping things together until they "work" and >>> calling it a day. >>> * One of the things I hope to accomplish in the future is versioned >>> component tests to protect against backwards incompatible changes >>> which can't be detected via signature checking. We can't implement >>> that plan if the tests become entangled with the source code. >>> >>> As a result I think we need to completely rethink the approach to >>> documentation that you've taken here. Some alternatives we should >>> consider: >>> * Move mockMVC into component test and out of service. Why did you >>> put the documentation and component test code in service in the first >>> place? >>> * Create a separate documentation module which imports code from >>> component-test and service. >>> * Using Spring Fox instead of MockMVC. When I originally evaluated >>> REST documentation approaches, it was exactly this mixing of other >>> concerns into testing that caused me to reject MockMVC in the first >>> place. Tutorials make good tests but there are many, many good tests >>> which make terrible documentation. >>> >> >> I'm okay with the approach to documentation being rethought. >> >> >>> >>> The use of MockMVC came up a few months ago when a potential GSoC >>> intern suggested it. I had thought, based on your response, that you >>> understood why it doesn't work well with our architecture. (1) Which >>> is why I didn't respond to that thread myself. What changed? >>> >>> In general, when making architectural changes which change the pattern >>> of all of the services it's not a good idea to just dump them into all >>> the services and then ask for feedback. You create a lot of >>> unnecessary work for yourself and for your code reviewer. A better >>> approach is to make the change in the fineract-cn-template project >>> first and then ask for feedback. Better still would be to engage a >>> discussion on the mailing list before doing a major architectural >>> change like this. If I had known you were working on this, I could >>> have shared my thoughts before you started. >>> >> >> I'm sorry I didn't open a discussion here about documenting the APIs. >> I'm hoping we'll continue that discussion on this thread and fix the >> mistakes I made. >> >>> >>> Best Regards, >>> Myrle >>> >>> 1.) https://lists.apache.org/thread.html/cc23a47229c262afbb6e474 >>> 9b7eec3117d084f84144877d59098713d@%3Cdev.fineract.apache.org%3E >>> 2.) https://lists.apache.org/thread.html/d0fbdb5b21b916cc8dd3c2a >>> 5061d3e90aa554bb74b63f8deabe69fe6@%3Cdev.fineract.apache.org%3E >> >> >> At Your Service, >> Isaac Kamga. >> >>> >>> >>> On Mon, Apr 23, 2018 at 8:42 PM, Isaac Kamga <[email protected]> >>> wrote: >>> > Hello everyone, >>> > >>> > Trust that you're doing great and congratulations to our recently >>> selected >>> > GSoC 2018 students. >>> > >>> > As you may have observed from Gitbox notifications, I have used the >>> MockMvc >>> > flavour of Spring Rest Docs <https://projects.spring.io/sp >>> ring-restdocs/> to >>> > document the APIs for most Fineract CN domain services viz identity, >>> > office, customer, group, accounting, deposit, payroll and teller, >>> created >>> > and merged pull requests. There were failing tests in portfolio ( as I >>> > earlier highlighted last week on the list ) and so snippets weren't >>> > generated to be used for documentation. >>> > >>> > After updating the affected repositories, you can view the >>> documentation >>> > for each of the services by opening the >>> > service/src/doc/html5/html5/api-docs.html file. >>> > >>> > I happily await your feedback and enhancements. >>> > >>> > At Your Service, >>> > Isaac Kamga. >>> >> >> >
