I'm +1 for moving right away to internal. Ludovic
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 ? >> >> Yes. > > 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. > > WDYT? > > Thanks > -Vincent > > PS: I don't personally know much at all about the REST API so I'm basing my > comments purely on Fabio's analysis. > >>> Ludovic >>> >>> 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" >>>>> >>>>> (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. >>>> >>>> >>>> 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. > _______________________________________________ > devs mailing list > devs@xwiki.org > 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 devs@xwiki.org http://lists.xwiki.org/mailman/listinfo/devs
