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 ? > 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
