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