Thanks to your advices (Eddy and Thomas), I propose you a clean version: https://github.com/gdelhumeau/xwiki-platform/tree/new-wiki-api/xwiki-platform-core/xwiki-platform-wiki/xwiki-platform-wiki-api/src/main/java/org/xwiki/wiki
I have only changed the API, not the implementation. I think it starts to be clean. 2013/10/7 Guillaume "Louis-Marie" Delhumeau <[email protected]> > Hi Eddy. > > First, thanks for your answer. > > 2013/10/7 Eduard Moraru <[email protected]> > >> 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; >> > > I did not really think about it, I just take the work done by Vincent on > wiki descriptors. > > >> 2) WikiManager: >> >> The methods wikiIdExists and and isWikiIdAvailable do the same thing, they >> just call it differently. Use only one, like wikiExists(). >> > > I should have added the javadoc about that. This is what I have in mind : > - wikiExists(String wikiOd) as you said > - wikiAvailable(String wikiId) that checks if the wiki id is valid and > free (that means that there is no existing database with that name -- not > necessary related to XWiki). > > It is not implemented yet. > > >> >> 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... >> > > I agree. I'll change it ;) > > >> >> 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. >> > > Indeed, but a Wiki class would not contain something else than a > descriptor. I don't know what to do. Renaming WikiDescriptor to Wiki? > > >> >> 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. >> > > I have keeped them because it is used in the descriptors implementation of > Vincent. But I agree that it is weird to have it in the API. > > >> >> 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-platform-core/xwiki-platform-wiki/xwiki-platform-wiki-default/src/main/java/org/xwiki/wiki/descriptor/internal/DefaultWikiManager.java#L209 >> > > I disagree. > In this code, I need to have the document related to the descriptor. It > depends on the current implementation, because we can imagine an > implementation where the descriptors are not stored in the wiki. So I won't > add a getDocument() in the API, but I need it in my implementation. > > >> >> 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-platform-core/xwiki-platform-wiki/xwiki-platform-wiki-default/src/main/java/org/xwiki/wiki/descriptor/internal/DefaultWikiManager.java#L126 >> > > I agree, I will move that code into WikiDescriptorBuilder. > > >> >> 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. >> > > OK. > > >> >> 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. > > > It is a temporary solution in my implementation. > The problem is: who has the right to remove a wiki ? > - his owner? But we don't have the notion of owner in this API. It belongs > to the wiki-users module to handle this concept. > - someone who has the CREATE_WIKI right? That means every person who has > the CREATE_WIKI right can also delete any existing wiki? > - someone who as the DELETE_WIKI right? Why not, but we need to make a > vote for adding this new right. > > So, in my first implementation, I have used the admin right. > > >> Also, consider the workspaces use case when user >> create and delete wikis/workspaces when they want to and when they are >> done >> with them. >> > > To me, it belongs to the future wiki-user module. > > >> - 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"? >> >> > It's a bad commit. I have setted this name for debugging reason. > > >> >> 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. >> > > The idea is to split in different simple modules, that enable us to easily > add a new feature or to depends on a module without depending on the whole > core. I try to reduce the dependencies while I design the modules. > > >> >> Hope this helps, >> Eduard >> >> > Thanks Eduard! > > Louis-Marie > > >> >> On Mon, Oct 7, 2013 at 11:49 AM, Guillaume "Louis-Marie" Delhumeau < >> [email protected]> wrote: >> >> > Hi. >> > >> > In this thread, I want to propose you the tiniest API that we need to >> > handle multiwiki. All other features (users, templates, workspaces...) >> will >> > be on other modules, because I think it is better for the extensibility. >> > >> > The new module will be based on the xwiki-platform-wiki-descriptor-api >> > module, that I will rename to xwiki-platform-wiki-api. The >> > WikiDescriptorManager becomes WikiManager and handle both descriptors >> and >> > databases. >> > >> > This API will permit: >> > * to create a wiki >> > * to remove a wiki >> > * to list all wikis (returning a list of descriptors) >> > * ... >> > >> > You can see the code of that proposal there: >> > >> > >> https://github.com/gdelhumeau/xwiki-platform/tree/new-wiki-api/xwiki-platform-core/xwiki-platform-wiki >> > >> > The most important is to decide what the API must look like. The >> > implementation can still be modified afterwards. >> > >> > I hope you like it, >> > >> > Louis-Marie >> > _______________________________________________ >> > devs mailing list >> > [email protected] >> > http://lists.xwiki.org/mailman/listinfo/devs >> > >> _______________________________________________ >> devs mailing list >> [email protected] >> http://lists.xwiki.org/mailman/listinfo/devs >> > > _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
