On Thu, Nov 17, 2011 at 1:27 PM, Denis Gervalle <[email protected]> wrote:
> Well, seems GitHub has misinterpreted what I have done, but if you follow
> the course of master, there is a simple commit that merge my branch to
> master, and you may see summary of what happens to master by following:
>
> https://github.com/xwiki/xwiki-platform/compare/615202482e...0edbb5d906
>
> Seems that the parents are not in the correct order and this has confused
> Github, I will be more careful next time.
>
> On Thu, Nov 17, 2011 at 12:30, Caleb James DeLisle <[email protected]
>> wrote:
>
>>
>> On 11/17/2011 05:56 AM, Vincent Massol wrote:
>> > Hi Denis,
>> >
>> > Just looked at the merged code (btw I can't find how to comment on it on
>> github, it's a big mess), so I'm commenting here:
>> >
>> > I noticed 2 issues:
>> >
>> > * One is that even though I said in the vote mail that I didn't agree
>> about keeping parameters in EntityReferences you've still kept them. Again
>> I think we shouldn't keep them because we don't have an agreement about
>> them ATM (Caleb is against them). Also you've exposed them with a protected
>> method which means users can extend EntityReference and use them. As I said
>> FTM I think you should move the notion of Locale **only** in
>> DocumentReference since this is the only thing we've agreed about so far.
>>
>> I agree that ideally locale would be set as a private final field in
>> DocumentReference.
>>
>
> Ideally, we should have not vote something on which we seems to disagree
> now.
> I will not revert my changes about parameters on EntityReference, since
> this has been voted.
> If we later decide (through a vote) that we do not want it, anyone will be
> free to remote them :)
>
>
>> I suggested switching it to protected since they are dangerous (at risk
>> for being removed entirely) and that would offer some protection while we
>> decide what to do.
>>
>
> This what I have done, now the feature is mostly hidden, since only
> protected methods expose them.
> I could have gone further by using package-scope is you really think this
> is better.
>
>
>> It is worth noting that there are a lot of dangerous protected methods
>> including ones which violate the immutability of the reference, something
>> else which I would ideally like to see done away with.
>>
>
> Immutability of the reference is not ensured in the reference package
> itself, and this is for me absolutely normal and probably the easiest way
> to provide "typed" references. I also like to extend that to the resolver,
> since these would be faster working on a mutable reference. I do not see
> this to be dangerous.
>
>
>>
>> Github seems to be displaying the merge incorrectly, you can see the
>> original commits here:
>> https://github.com/xwiki/xwiki-platform/commits/feature-immutable-refs
>
>
> Yes, since I have not deleted the branch, should I ?
Yes since you merged it, we usually remove completed branches.
>
>
>> Caleb
>>
>> >
>> > * You have some comments in DocumentReference like this:
>> >
>> > * @param locale the new locale for this reference, if null, locale
>> is removed
>> >
>>
>> I don't understand what this means. How can it be removed since they are
>> immutable and it's in a constructor :)
>>
>
> This constructor create a new reference from another one, replacing the
> Locale. If the locale is null, their will be no locale, compare to the
> initial reference that may have one. Nothing wrong here.
>
>
>> >
>> > Also looking at setLocale I see:
>> >
>> > protected void setLocale(Locale locale)
>> > {
>> > if (locale != null) {
>> > setParameter(LOCALE, locale);
>> > }
>> > }
>>
>
> You catch it :) I have missed this correction following a review by Thomas,
> the if should not be there, I will remove it.
>
>
>> >
>> > I don't see any removal.
>>
>
> Of course, it is immutable !
>
>
>> >
>> > Is it a leftover/typo?
>> >
>> > Thanks
>> > -Vincent
>> >
>> > _______________________________________________
>> > devs mailing list
>> > [email protected]
>> > http://lists.xwiki.org/mailman/listinfo/devs
>> >
>>
>> _______________________________________________
>> devs mailing list
>> [email protected]
>> http://lists.xwiki.org/mailman/listinfo/devs
>>
>
>
>
> --
> Denis Gervalle
> SOFTEC sa - CEO
> eGuilde sarl - CTO
> _______________________________________________
> 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
