git.net

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Apache Fineract CN API Documentation


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.

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?

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:

* 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.

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.

Best Regards,
Myrle

1.) https://lists.apache.org/thread.html/cc23a47229c262afbb6e4749b7eec3117d084f84144877d59098713d@%3Cdev.fineract.apache.org%3E
2.) https://lists.apache.org/thread.html/d0fbdb5b21b916cc8dd3c2a5061d3e90aa554bb74b63f8deabe69fe6@%3Cdev.fineract.apache.org%3E

On Mon, Apr 23, 2018 at 8:42 PM, Isaac Kamga <isaac.kamga@xxxxxxxxx> 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/spring-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.