Add IContextFunction.ServiceContextKey OSGi component property type #1577
Conversation
| */ | ||
| @ComponentPropertyType | ||
| @Retention(RetentionPolicy.SOURCE) | ||
| @interface ServiceContextKey { |
There was a problem hiding this comment.
Any reason to not make it a top level class?
There was a problem hiding this comment.
Nesting it makes it more obvious that ServiceContextKey is only relevant for an implementation of the surrounding interface, in this case IContextFunction. It indicates the context better, instead to when it is a top-level class. Then one can think more easy that it's independently useful.
The SERVICE_CONTEXT_KEY string constant is defined here as well.
There was a problem hiding this comment.
I think this is rather unusual, I never seen this in any other (e.g. OSGi) defined property types, so I would suggest to make it a top-level class.
There was a problem hiding this comment.
It wouldn't be the first one:
There was a problem hiding this comment.
I think this is rather unusual, I never seen this in any other (e.g. OSGi) defined property types, so I would suggest to make it a top-level class.
I haven't seen it often too, but in case we all agree that it's a good idea, why not do it differently? :)
What do you think is the advantage of having it as a top-level class?
From my POV having it nested has the advantage that the context is more clear.
It wouldn't be the first one:
It might be that I got the idea from that a while ago already (and forgot the source).
There was a problem hiding this comment.
It wouldn't be the first one:
Eclipse Testing framework is probably not the beste example...
So why not take as an example the other already existing property type in platform?
Or even how OSGi itself does it (spoiler it is using dedicated types).
For me "inner interfaces" have really no added value at all (beside they safe a javafile) except one plant to have multiple ones with the same name (what makes no sense in property types), e.g. if one has an Interface + a Builder interface or like java do with Entry and want to have multiple ones in the same package.
But for me this more leads to confusion due to the name clashes (I use one framework that makes use of it and its just horrible) even the java case is annoying (e.g. JDT suggest mes always the "wrong" first and I have to carefully inspect what really to import).
I also don't think it really makes things more "clear" ist just harder to discover as it is hidden inside the rest of the source code. Most of the time where I used such construct I later refactor it to being a top-level interface for several reasons.
There was a problem hiding this comment.
Or even how OSGi itself does it (spoiler it is using dedicated types).
In OSGi all ComponentPropertyType implementations are placed in a dedicated propertytypes because there are many of them for each topic:
https://github.com/search?q=repo%3Aosgi%2Fosgi+import+org.osgi.service.component.annotations.ComponentPropertyType&type=code
But as far as I can see it, no property-type is colocated in the same package to the targeted service interface.
However, for one type I don't think it makes sense to add a new package.
So why not take as an example the other already existing property type in platform?
That's the only existing property-type annotation I found in the equinox, eclipse-platform, JDT and PDE organization and you added it that way in
I know I also reviewed that PR back than and didn't suggest the same, but my point is, that with one occurrence I wouldn't say it a well established style that is used commonly and very often all over Eclipse. To me this seems to be more an open field and if this is submitted as it is we have a 50:50 split. So maybe this could be considered as an experiment to see what works better on the long run?
But for me this more leads to confusion due to the name clashes (I use one framework that makes use of it and its just horrible) even the java case is annoying (e.g. JDT suggest mes always the "wrong" first and I have to carefully inspect what really to import).
But for the this inner interface we have no name clash and if it would clash with another class you always have to check what is exactly imported, because you cannot be sure it's the right one, just because it's a top-level class?
I even did some small experiment to check how JDT proposes the annotations as a top-level and nested annotation-interface and there seems to be no difference:
I also don't think it really makes things more "clear" ist just harder to discover as it is hidden inside the rest of the source code.
This could happen in same cases but also the opposite could happen: One looks into the interface and then sees this property-type. And without that wouldn't have discovered it that person doesn't scan the entire package. Of course it could also be documented accordingly, but summed up we would probably have more documentation to mention the property-type in the service and reference back to the service in the property-type. With a nested annotation/interface it would be (at least partly) be implied by the 'context'.
But I think we are starting to turn in circles and each one of us has a clear but unfortunately opposite opinion on that. Still I think neither one is wrong, both are valid approach, it's just the question which style is better.
So I would like to ask others what their opinion is and what they think is overall better?
There was a problem hiding this comment.
I'm very biased. I do a lot of nesting to keep the concepts that used together close together:
This annotation simplifies the specification of the 'service.context.key' service property for IContextFunction implementations and makes it type-safe and more robust: ''' @component(service = IContextFunction.class) @IContextFunction.ServiceContextKey(IProgressService.class) public class ProgressServiceCreationFunction extends ContextFunction { '''
HannesWell
force-pushed
the
add-IContextFunction-ServiceContextKey-componentPropertyType
branch
from
c4cf1be
to
ec03a19
Compare
This annotation simplifies the specification of the 'service.context.key' service property for IContextFunction implementations and makes it type-safe and more robust: