Re: [xwiki-devs] [Proposal] Wiki API

Hi Guilllaume, 1) What is the point of the WikiDescriptorAlias class if it just holds and returns a string? Also, the only method in the WikiDescriptorManager that works with aliases accepts a String instead of the WikiDescriptorAlias class, so this means that the class is useless: WikiDescriptor getByWikiAlias(String wikiAlias) throws WikiManagerException; 2) WikiManager: The methods wikiIdExists and and isWikiIdAvailable do the same thing, they just call it differently. Use only one, like wikiExists(). Also, since it's a WikiManager, so the "wiki" part is in the name of the class, we get it that it deals with wikis. You could remove the "wiki" part from method names, like: - create instead of createWiki - delete instead of deleteWiki - exists instead of existsWiki - so on... Note on the naming of methods: I`ve noticed a rather annoying thing in the way the methods are named. So you have a class that is say... WikiDescriptor. 3) WikiManager: What is the point of these 2 methods? void setDescriptor(WikiDescriptor descriptor); void removeDescriptor(WikiDescriptor descriptor); Looking at the implementation, I see that you`re handling descriptors (setting/deleting) whithout corelating it to the wikis. I mean, if you remove a descriptor, then you must remove the wiki too, and that is the task of the WikiManager.delete() method. 4) Don`t use implementation details. Improve the interface (API) if there is information you`re missing: https://github.com/gdelhumeau/xwiki-platform/blob/new-wiki-api/xwiki-platfo… 5) DefaultWikiManager.createDescritptor Since you have a WikiDescriptorBuilder class, maybe that is where the logic of creating/building the descriptor's details (objects, document) and where the logic of extracting the document name from wiki name and viceversa should be located. https://github.com/gdelhumeau/xwiki-platform/blob/new-wiki-api/xwiki-platfo… 6) DefaultWikiManager.getByWikiId Again seeing the logic of wikiID-to-document. It should be located in a single place and not spread in multiple places. Use a method of the WikiManager class or of the DescriptorBuilder class. 7) WikiManagerScriptService: - deleteWiki: Why do you need Admin right to delete a wiki when you did not need it in order to create it? That`s bad. We should probably have a deleteWiki right as well. Also, consider the workspaces use case when user create and delete wikis/workspaces when they want to and when they are done with them. - context key for exceptions "lastexception2" is not the best of choices. Try something like "wikiException" instead, or even just "lastexception" like it was before (if we consider this as a best practice). What's the reason for "lastexception2"? As a general note, I fail to see the added value of this module at this point. I mean, the whole point was to have both the functionality of WikiManager and Workspaces. I get it that it's a work in progress and that it's at the first stages, but you need to touch those points before it becomes relevant. Not that it's bad, just that it's not much there to see at this point. Hope this helps, Eduard On Mon, Oct 7, 2013 at 11:49 AM, Guillaume "Louis-Marie" Delhumeau < gdelhumeau(a)xwiki.com&gt; wrote: