17 Nov
2011
17 Nov
'11
11:56 a.m.
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.
* 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 :)
Also looking at setLocale I see:
protected void setLocale(Locale locale)
{
if (locale != null) {
setParameter(LOCALE, locale);
}
}
I don't see any removal.
Is it a leftover/typo?
Thanks
-Vincent
17 Nov
17 Nov
12:30 p.m.
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.
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.
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.
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
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 :)
Also looking at setLocale I see:
protected void setLocale(Locale locale)
{
if (locale != null) {
setParameter(LOCALE, locale);
}
}
I don't see any removal.
Is it a leftover/typo?
Thanks
-Vincent
_______________________________________________
devs mailing list
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs
1:27 p.m.
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 <calebdelisle(a)lavabit.com
You catch it :) I have missed this correction following a review by Thomas,
the if should not be there, I will remove it.
Of course, it is immutable !
--
Denis Gervalle
SOFTEC sa - CEO
eGuilde sarl - CTO
wrote:
On 11/17/2011 05:56 AM, Vincent Massol wrote:
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 :)
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.
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
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.
* You have some comments in DocumentReference like this:
* @param locale the new locale for this reference, if null, locale
is
removed
Also looking at setLocale I see:
protected void setLocale(Locale locale)
{
if (locale != null) {
setParameter(LOCALE, locale);
}
}
I don't see any removal.
Is it a leftover/typo?
Thanks
-Vincent
_______________________________________________
devs mailing list
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs
_______________________________________________
devs mailing list
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs
1:39 p.m.
On Thu, Nov 17, 2011 at 1:27 PM, Denis Gervalle <dgl(a)softec.lu> wrote:
You catch it :) I have missed this correction following a review by Thomas,
the if should not be there, I will remove it.
Of course, it is immutable !
--
Denis Gervalle
SOFTEC sa - CEO
eGuilde sarl - CTO
_______________________________________________
devs mailing list
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs
--
Thomas Mortagne
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 <calebdelisle(a)lavabit.com
Yes since you merged it, we usually remove completed branches.
wrote:
On 11/17/2011 05:56 AM, Vincent Massol wrote:
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 :)
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.
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
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.
* You have some comments in DocumentReference like this:
* @param locale the new locale for this reference, if null, locale
is
removed
Also looking at setLocale I see:
protected void setLocale(Locale locale)
{
if (locale != null) {
setParameter(LOCALE, locale);
}
}
I don't see any removal.
Is it a leftover/typo?
Thanks
-Vincent
_______________________________________________
devs mailing list
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs
_______________________________________________
devs mailing list
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs
4687
days inactive
4687
days old
3 comments
4 participants
participants (4)
-
Caleb James DeLisle -
Denis Gervalle -
Thomas Mortagne -
Vincent Massol