git.net

[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
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/cc23a47229c262afbb6e4749b7eec3
> 117d084f84144877d59098713d@%3Cdev.fineract.apache.org%3E
> 2.) https://lists.apache.org/thread.html/d0fbdb5b21b916cc8dd3c2a5061d3e
> 90aa554bb74b63f8deabe69fe6@%3Cdev.fineract.apache.org%3E


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