Prologue: I'm pretty indifferent on the structure of the JavaHL packages and such. I just help maintain the JNI glue, so I've approached these changes from a JNI/C++ perspective.
On Feb 12, 2010, at 8:08 AM, Mark Phippard wrote:
> Renaming the JavaHL classes and providing compatibility wrappers for
> the old classes has allowed us to make some changes in the new
> classes, such as dropping deprecated classes and renaming things etc.
> I took the opportunity to ask the SVNKit developers if there was
> anything more we could do to help them. They provide a pure-Java
> implementation of the JavaHL interface and I know they ran into a few
> areas in our code where we did things that made this difficult.
>
> They provided me the attached patch and accompanying explanation of
> the changes. It all makes sense to me, but I thought I would run it
> by the list. A lot of these items, such as creating an interface for
> SVNAdmin, are really obvious. Maybe the constructor change will be
> more controversial, but I think it reflects what needs to be done if
> the JavaHL interface is truly meant to be something that could have
> alternate implementations.
>
> Here is there explanation of the changes in the patch:
>
> 1. Changed all package local constructors to public ones. This will let
> SVNKit keep as much of the SVNKit-code out of the Subversion namespace
> as possible. Also, current version is not consistent in that aspect, for
> instance Status class constructor is public while Info2 class has
> package local one.
I'm all for consistency, but the advantage of having private constructors is that we can still access them from JNI (which theoretically is the only thing using them), but don't have to maintain older versions of them for backward compat. If that theory isn't correct, and we chose to make them public, I suppose that's okay.
> 2. Added ISVNAdmin interface and made SVNAdmin implement it.
>
> As you know, there are two ways SVNKit could be used:
>
> 1. User removes svn-javahl.jar and replaces it with svnkit-javahl.jar.
> In this case original JavaHL classes are replaced with SVNKit ones, in
> particular SVNClient, SVNAdmin, Path and (in this new version) Version
> and NativeResources. This is not preferred way to use SVNKit and I would
> talk of it as of a deprecated one. With that approach it is not possible
> to switch between implementations without use of a custom class loader.
>
> 2. Another, preferred approach, is to work with JavaHL API using its
> interfaces (ISVNClient) and only depend on particular implementation
> where instance of certain implementation is instantiated.
>
> So, to enable second approach for users who'd like to use SVNAdmin I've
> added ISVNAdmin interface and made SVNAdmin class implement it. I also
> moved constant and callback interface defined in the SVNAdmin class to
> the new ISVNAdmin interface.
>
> 3. NativeResources.version field made private and instead
> NativeResources.getVersion() method added.
>
> This modification will let SVNKit provide a replacement of
> NativeResources class for the "deprecated" approach along with custom
> getVersion() method implementation.
4. I had to leave a couple of the deprecated functions in the new client class, and would really like to see them removed. For instance, info() vs. info2(). I'm not sure when they've never been wrapped, but if one of the Java gurus could take a look at implementing the former as a wrapper in the old class, that would make the new class a bit more lean.
> Please let me know what you think on my proposal and whether it will be
> possible to include this patch into the new version of the API.
Aside from the above, I'm generally in favor. I have not reviewed the patch.
-Hyrum
Received on 2010-02-12 18:14:33 CET