Before writing extensions the API need to be extensible and we don't see it in your current API ;) This needs to be there before validating this API.
On Mon, Oct 7, 2013 at 5:17 PM, Guillaume "Louis-Marie" Delhumeau <[email protected]> wrote: > 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 -- Thomas Mortagne _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
