|Ludovic Dubost||Jul 27, 2012 1:12 am|
|Ludovic Dubost||Aug 7, 2012 10:33 pm|
|Fabio Mancinelli||Aug 21, 2012 5:18 am|
|Sergiu Dumitriu||Aug 21, 2012 6:19 am|
|Ludovic Dubost||Aug 21, 2012 1:02 pm|
|Vincent Massol||Aug 21, 2012 1:17 pm|
|Fabio Mancinelli||Aug 21, 2012 1:48 pm|
|Sergiu Dumitriu||Aug 21, 2012 1:56 pm|
|Vincent Massol||Aug 21, 2012 2:05 pm|
|Ludovic Dubost||Aug 21, 2012 3:54 pm|
|Thomas Mortagne||Aug 25, 2012 8:40 am|
|Ludovic Dubost||Oct 8, 2012 7:35 am|
|Ludovic Dubost||Oct 10, 2012 7:37 am|
|Fabio Mancinelli||Oct 12, 2012 8:59 am|
|Jerome Velociter||Oct 12, 2012 11:09 am|
|Thomas Mortagne||Oct 14, 2012 1:55 am|
|Fabio Mancinelli||Oct 17, 2012 2:14 pm|
|Fabio Mancinelli||Oct 19, 2012 11:24 am|
|Ludovic Dubost||Oct 19, 2012 1:04 pm|
|Subject:||Re: [xwiki-devs] [VOTE] REST API Changes not passing CLIRR. Ignore errors|
|From:||Ludovic Dubost (ludo...@xwiki.com)|
|Date:||Aug 21, 2012 3:54:18 pm|
I'm +1 for moving right away to internal.
2012/8/21 Vincent Massol <vinc...@massol.net>:
On Aug 21, 2012, at 10:56 PM, Sergiu Dumitriu wrote:
On 08/21/2012 04:02 PM, Ludovic Dubost wrote:
So we should add methods to keep the old methods available and mark all of them/the whole class deprecated ?
Well since Fabio says that these classes should have been internal from the
beginning and it was a mistake and since Fabio thinks that it's unlikely that
anybody would be using them we could make an exception and skip the deprecation
and move them to the internal package.
PS: I don't personally know much at all about the REST API so I'm basing my
comments purely on Fabio's analysis.
2012/8/21 Sergiu Dumitriu <ser...@xwiki.com>:
On 08/21/2012 08:18 AM, Fabio Mancinelli wrote:
On Fri, Jul 27, 2012 at 10:12 AM, Ludovic Dubost <ludo...@xwiki.com> 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.
+1 as well.
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.
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"
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:
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.
I'd rather not keep the old methods, but since this is a public class, it needs to follow our deprecation strategy, so +1 for Fabio's suggestion.