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

Re: Apache Fineract CN API Documentation

Hi Myrle,

Thanks for your elaborate feedback and corrections.

On Wed, Apr 25, 2018 at 9:10 AM, Myrle Krantz <myrle@xxxxxxxxxx> 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

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

At Your Service,
Isaac Kamga.

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

( ! ) Warning: include(msgfooter.php): failed to open stream: No such file or directory in /var/www/git/apache-fineract-developers/msg04205.html on line 223
Call Stack
10.0008372856{main}( ).../msg04205.html:0

( ! ) Warning: include(): Failed opening 'msgfooter.php' for inclusion (include_path='.:/var/www/git') in /var/www/git/apache-fineract-developers/msg04205.html on line 223
Call Stack
10.0008372856{main}( ).../msg04205.html:0