Thanks for taking discussion to the email list - I had to break up your pull request as it included a fix (to be back ported) and an API change.
You have two API changes in the works right - a request to create a directory (something like resource.dir()?) along with making resource.delete() recursive. So yeah if you feel that implementing resource.delete() is easier to implement - perhaps by calling to ResourceStore.remove( path ) - then I am fine to make the change. We can also remove Resources.delete( resource ) as I do not want us maintaining more than one way to do things. We should also make the return value consistent. The return value was chosen for consistency with File.delete(). Much like making Resource method compatible with File, the return value made it easier to migrate code that made direct use of File. A question about resource.delete() being much more efficient to implement, are you checking (and acquiring) all the resource locks before deleting? With respect to creating a directory - I want to avoid putting developers in the situation where the API is forcing them to create a directory using boilerplate code prior creating files. Your earlier example showed directory creation as a placeholder (almost like a sentinel file) so I was not quite sure if it was a poor example or if you had an actual use case. -- Jody -- Jody Garnett On 12 August 2015 at 03:20, Niels Charlier <[email protected]> wrote: > Hi Jody, > > To follow up on the github discussion regarding the resources delete API. > As I understand we have three methods to delete resources: > > 1. Resource.delete() - non recursive > (until last PR), returns false if file doesn't exist > 2. ResourceStore.remove (String path) - recursive, returns true if > file does exist > 3. Resources.delete(Resource res) - recursive, returns false > if file doesn't exist > > I understand that the idea of creating (3) is that it would factor out the > recursive algorithm, leaving to implementers of resource stores only the > need to implement the deletion of one resource / empty directory. But then > (2) is still recursive, which enforces an implementation of the recursion > in the resource store anyway. > > I understand the reasoning, but recursive delete can be made much more > efficiently for the JDBCStore than the implementation in (3) with less > queries sent to the database. I think that resource stores should be > allowed to have their own implementation for that reason. Perhaps one > future implementation of resource store could delete a full directory in > one go, without the need for recursion. > > So yes, I'd agree to get rid of the (3) method all together and change the > contract. > > Another thing I'd like to see changed is that I'd like the rule on whether > to return true or false on a non-existing file should be the same for (1) > and (2). I don't care which is chosen, but since method (2) is hardly (if > anywhere at all) used, it would be least risky to change it. Unless there > was/is a conscious reason to do it this way, of course. It should be better > documented then though, because at this moment the behaviour towards > non-existing files for (1) is not documented, which may lead people to > believe that it will the same as (2), which is explicitly documented. > > Regards > Niels >
------------------------------------------------------------------------------
_______________________________________________ Geoserver-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/geoserver-devel
