On 10/12/2012 05:59 PM, Fabio Mancinelli wrote:
On Mon, Oct 8, 2012 at 4:35 PM, Ludovic Dubost <[email protected]> wrote:
Hi,
All votes told to move the code to internal packages.
This is something I can do, however I'm not sure exactly which code
should go into the internal package and which one should stay where it
is.
Right now here is the list of classes failing or not the CLIRR tests
Not failing:
org.xwiki.rest.ComponentsObjectFactory
org.xwiki.rest.Constants
org.xwiki.rest.RangeIterable
org.xwiki.rest.Relations
org.xwiki.rest.Utils
org.xwiki.rest.XWikiAuthentication
org.xwiki.rest.XWikiJaxRsApplication
org.xwiki.rest.XWikiResource
org.xwiki.rest.XWikiRestComponent
org.xwiki.rest.XWikiRestletJaxRsApplication
org.xwiki.rest.XWikiRestletServlet
org.xwiki.rest.XWikiSecretVerifier
org.xwiki.rest.XWikiSetupCleanupFilter
org.xwiki.rest.exceptions.QueryExceptionMapper
org.xwiki.rest.exceptions.XWikiExceptionMapper
org.xwiki.rest.representations.TextPlainReader
org.xwiki.rest.representations.comments.FormUrlEncodedCommentReader
org.xwiki.rest.representations.comments.TextPlainCommentReader
org.xwiki.rest.representations.objects.FormUrlEncodedObjectReader
org.xwiki.rest.representations.objects.FormUrlEncodedPropertyReader
org.xwiki.rest.representations.objects.TextPlainPropertyReader
org.xwiki.rest.representations.pages.FormUrlEncodedPageReader
org.xwiki.rest.representations.pages.TextPlainPageReader
org.xwiki.rest.representations.tags.FormUrlEncodedTagsReader
org.xwiki.rest.representations.tags.TextPlainTagsReader
org.xwiki.rest.resources.BrowserAuthenticationResource
org.xwiki.rest.resources.RootResource
org.xwiki.rest.resources.SyntaxesResource
org.xwiki.rest.resources.attachments.AttachmentAtPageVersionResource
org.xwiki.rest.resources.attachments.AttachmentHistoryResource
org.xwiki.rest.resources.attachments.AttachmentResource
org.xwiki.rest.resources.attachments.AttachmentVersionResource
org.xwiki.rest.resources.classes.ClassPropertiesResource
org.xwiki.rest.resources.classes.ClassPropertyResource
org.xwiki.rest.resources.classes.ClassResource
org.xwiki.rest.resources.classes.ClassesResource
org.xwiki.rest.resources.objects.BaseObjectsResource
org.xwiki.rest.resources.pages.ModifiablePageResource
org.xwiki.rest.resources.pages.PageTagsResource
org.xwiki.rest.resources.pages.PageTranslationsResource
org.xwiki.rest.resources.spaces.SpaceResource
org.xwiki.rest.resources.spaces.SpacesResource
org.xwiki.rest.resources.tags.TagsResource
org.xwiki.rest.resources.wikis.WikiPagesResource
org.xwiki.rest.resources.wikis.WikiSearchQueryResource
org.xwiki.rest.resources.wikis.WikisResource
Failing:
org.xwiki.rest.DomainObjectFactory
org.xwiki.rest.resources.BaseAttachmentsResource
org.xwiki.rest.resources.BaseSearchResult
org.xwiki.rest.resources.ModificationsResource
org.xwiki.rest.resources.attachments.AttachmentsAtPageVersionResource
org.xwiki.rest.resources.attachments.AttachmentsResource
org.xwiki.rest.resources.comments.CommentResource
org.xwiki.rest.resources.comments.CommentVersionResource
org.xwiki.rest.resources.comments.CommentsResource
org.xwiki.rest.resources.comments.CommentsVersionResource
org.xwiki.rest.resources.objects.AllObjectsForClassNameResource
org.xwiki.rest.resources.objects.ObjectAtPageVersionResource
org.xwiki.rest.resources.objects.ObjectPropertiesAtPageVersionResource
org.xwiki.rest.resources.objects.ObjectPropertiesResource
org.xwiki.rest.resources.objects.ObjectPropertyAtPageVersionResource
org.xwiki.rest.resources.objects.ObjectPropertyResource
org.xwiki.rest.resources.objects.ObjectResource
org.xwiki.rest.resources.objects.ObjectsAtPageVersionResource
org.xwiki.rest.resources.objects.ObjectsForClassNameResource
org.xwiki.rest.resources.objects.ObjectsResource
org.xwiki.rest.resources.pages.PageChildrenResource
org.xwiki.rest.resources.pages.PageHistoryResource
org.xwiki.rest.resources.pages.PageResource
org.xwiki.rest.resources.pages.PageTranslationHistoryResource
org.xwiki.rest.resources.pages.PageTranslationResource
org.xwiki.rest.resources.pages.PageTranslationVersionResource
org.xwiki.rest.resources.pages.PageVersionResource
org.xwiki.rest.resources.pages.PagesResource
org.xwiki.rest.resources.spaces.SpaceAttachmentsResource
org.xwiki.rest.resources.spaces.SpaceSearchResource
org.xwiki.rest.resources.tags.PagesForTagsResource
org.xwiki.rest.resources.wikis.WikiAttachmentsResource
org.xwiki.rest.resources.wikis.WikiSearchResource
So which one of these are not internal ?
I think that everything except XWikiResource and Relations should be
moved to internal.
These are implementations of the HTTP actions on different resources
and the implementation of content-type conversions. Everything that is
actually internal.
Developers who wants to implement new resources only needs
XWikiRestComponent, XWikiResource (which provides an extensible class
with some utility methods) and XWikiRelations to have a list of the
relations that can be specified in <link>s
So I am going to move everything else in internal.
Sounds good, +1
Jerome
-Fabio
Ludovic
2012/8/25 Thomas Mortagne <[email protected]>:
On Tue, Aug 21, 2012 at 10:18 PM, Vincent Massol <[email protected]> wrote:
On Aug 21, 2012, at 2:18 PM, Fabio Mancinelli wrote:
On Fri, Jul 27, 2012 at 10:12 AM, Ludovic Dubost <[email protected]> wrote:
As part of rest improvements to display pretty names of users and
other improvements, I'm getting CLIRR errors because of API changes of
the model and of public class:
1/ Model CLIRR error because the version field has been moved to
PageSummary from Page. Page extends PageSummary. I need the version
field also in representations sending back only PageSummaries.
Unfortunately CLIRR does not realize that the version field is still
there when moved to the super class. I believe it's safe to ignore
this error. Howerver I've put ignore all errors on the Page class as I
don't have a way to ignore this specific error
Yep, I think it's safe. We're adding stuff in a representation (page
summary) and keeping it also in the other, so API-wise it's ok.
Note that CLIRR doesn't have false positives so if it complains it means there's a
breakage. The only decision to take is whether it's an "acceptable" breakage,
i.e. we voluntarily break assuming that nobody was using it and accept that a few users
might get broken.
2/ CLIRR errors because of parameter additions to objects that are
used (I think) only internally by the REST server API. Here are the
errors:
[ERROR] org.xwiki.rest.DomainObjectFactory: In method 'public
org.xwiki.rest.model.jaxb.Attachment
createAttachment(org.xwiki.rest.model.jaxb.ObjectFactory,
java.net.URI, com.xpn.xwiki.api.Attachment, java.lang.String,
java.lang.String)' the number of arguments has changed
The DomainObjectFactory is actually a utility class that is used to
build REST-model objects from XWiki-model objects.
It has been created just to prevent code duplication in resource
implementations.
The question is whether this is supposed to be a SPI or not.
Now I think it's unlikely that somebody uses it outside the REST
module (a quick grep confirmed this for platform).
The only use case for a developer of a module to use this class is if
she wants to return a REST-model object and build it using the utility
methods.
I think this is quite unlikely.
AFAIU all parameters additions are about "pretty names"
(https://github.com/ldubost/xwiki-platform/compare/master...bd49bcc84e1dec3d20b57e970c293a459524d2e4#L3L81)
If we want to be conservative we might do the following: we can add
the new methods and preserve the old ones making them call the new
ones with default parameters.
* false in methods like this
https://github.com/ldubost/xwiki-platform/compare/master...bd49bcc84e1dec3d20b57e970c293a459524d2e4#L3L407
* null, false in methods like this
https://github.com/ldubost/xwiki-platform/compare/master...bd49bcc84e1dec3d20b57e970c293a459524d2e4#L3L494
. This implies that in the new implementation the if statement should
also check for null values (like in this case:
https://github.com/ldubost/xwiki-platform/compare/master...bd49bcc84e1dec3d20b57e970c293a459524d2e4#L3R629)
We could also think about whether continuing to keep this class in the
public API. It could make sense but I think that nobody will ever use
it so we can start to @deprecate it and eventually move it in internal
packages.
Based on what you say I'd say that these classes/methods were put public by
mistake and should all be moved to the internal package without going through
deprecation.
+1 for internal, if it's not supposed to be used outside of the module
that's the definition of internal
Of course this means no other other XWiki modules should use them either since
they're internal.
Also this means that we don't provide any SPI either since SPI are user-public.
Is that what we want?
Thanks
-Vincent
_______________________________________________
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
--
Ludovic Dubost
Founder and CEO
Blog: http://blog.ludovic.org/
XWiki: http://www.xwiki.com
Skype: ldubost GTalk: ldubost
_______________________________________________
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
