[Sightly] API vs. Implementation

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[Sightly] API vs. Implementation

Felix Meschberger-3
Hi

TL;DR We have to cleanup APIs and exports from Sightly Engine and JS UseProvider bundles.

I have been going through the Sightly Engine and JS UseProvider bundles.

The JS UseProvider bundle currently exports everything at the bundle version. I would think this bundle should not be exporting anything at all. To leverage the default bundle plugin setup, I suggest we move everything into an impl package such that the top level will be org.apache.sling.scripting.sightly.js.impl.

The case for the Sightly Engine is slightly more complicated. That bundle currently the .api package contains the exported API while the rest of the packages are implementations.

As with the JS UseProvider I suggest to move all implementation packages to below org.apache.sling.scripting.sightly.impl. This allows us to leverage the default setup of the bundle plugin to not export anything containing impl in the package name.

As for the API, I would like to propose to refactor a bit to create multiple smaller packages which are more coherent in their export:

o.a.s.scripting.sightly:
  - ObjectModel
  - Record
  - ResourceResolution
  - SightlyEngineException extends SlingException
  - SightlyParsingException extends SightlyEngineException
  - SightlyRuntime
  - StackedWriter

o.a.s.scripting.sightly.extension:
  - ExtensionInstance
  - RuntimeExtension
  - RuntimeExtensionComponent
  - RuntimeExtensionException extends SightlyEngineException

o.a.s.scripting.sightly.render:
  - BaseRenderUnit
  - RenderContext
  - RenderUnit
  - SightlyRenderException extends SightlyEngineException
  - UnitLocator

o.a.s.scripting.sightly.use:
  - ProviderOutcome
  - SightlyUseException extends SightlyEngineException
  - Use
  - UseProvider
  - UseProviderComponent

Apart from splitting the base .api package, this are further discussions:

* Exceptions are forming a hierarchy with SightlyEngineException being the root and itself extending from SlingException
* I wonder, whether we should really drop the *Component abstract classes (RuntimeExtensionComponent and UseProviderComponent). I don’t think they provide real value at all but have an activate method, which they expect to be called and which they expect to be a DS component annotated with the Felix annotations. I think this is brittle.
* UseProvider defines an ordering method and declares itself being Comparable. I think this is wrong. The UseProviders should be sorted using regular OSGi service ranking.
* I wonder, whether we really have to have all this API exported at all: Some API I don’t see implemented at all (e.g. BaseRenderUnit — or is that the base class for the Java classes generated from the Sightly templates ?)
* Last but not least (actually most important of all): There is close to no JavaDoc. Before cutting a release of this bundle, JavaDoc must be provided for the API. For example I am not sure, what the Record or the ObjectModel interfaces are really used for.

Thanks
Felix
Reply | Threaded
Open this post in threaded view
|

Re: [Sightly] API vs. Implementation

Radu Cotescu-2
Hi Felix,

The API reorganisation looks good to me.

On Wed, Nov 19, 2014 at 10:49 PM, Felix Meschberger <[hidden email]>
wrote:

> Apart from splitting the base .api package, this are further discussions:
>
> * Exceptions are forming a hierarchy with SightlyEngineException being the
> root and itself extending from SlingException
>

Sure, it makes sense.


> * I wonder, whether we should really drop the *Component abstract classes
> (RuntimeExtensionComponent and UseProviderComponent). I don’t think they
> provide real value at all but have an activate method, which they expect to
> be called and which they expect to be a DS component annotated with the
> Felix annotations. I think this is brittle.
>

The abstract classes were written specifically for that activate method, in
order to avoid writing those lines of code for every implementation.



> * UseProvider defines an ordering method and declares itself being
> Comparable. I think this is wrong. The UseProviders should be sorted using
> regular OSGi service ranking.
>

If we make the service.ranking property configurable (metatype = true) I
guess we could use the OSGi ranking. Theoretically the Use providers can be
configured to be called in a pre-determined order, depending on how the
Sightly projects from an instance make use of the Use-API (Java Use-API
objects deployed in bundles / Java Use objects near the component script in
the repo / JS Use-API, etc.). Having the Use providers called in the most
favourable way can increase performance for Sightly applications.


> * I wonder, whether we really have to have all this API exported at all:
> Some API I don’t see implemented at all (e.g. BaseRenderUnit — or is that
> the base class for the Java classes generated from the Sightly templates ?)
>

The BaseRenderUnit is indeed the base class for classes generated from
Sightly scripts, which is why we need to export the other classes as well.
In this regard Sightly is no different from the JSP engine.


> * Last but not least (actually most important of all): There is close to
> no JavaDoc. Before cutting a release of this bundle, JavaDoc must be
> provided for the API. For example I am not sure, what the Record or the
> ObjectModel interfaces are really used for.
>

It's on my TODO list.

Thanks,
Radu
Reply | Threaded
Open this post in threaded view
|

Re: [Sightly] API vs. Implementation

Felix Meschberger-3
Hi

Thanks for your reply.

For those interested in following, Radu and I are cooperating on these changes in my GitHub fork at https://github.com/fmeschbe/sling


> Am 19.11.2014 um 22:53 schrieb Radu Cotescu <[hidden email]>:
>
> Hi Felix,
>
> The API reorganisation looks good to me.
>
> On Wed, Nov 19, 2014 at 10:49 PM, Felix Meschberger <[hidden email]>
> wrote:
>
>> Apart from splitting the base .api package, this are further discussions:
>>
>> * Exceptions are forming a hierarchy with SightlyEngineException being the
>> root and itself extending from SlingException
>>
>
> Sure, it makes sense.

Done in the fork. The „root“ exception for Sightly is not SightlyException (SightlyEngineException renamed to SightlyException).

>
>
>> * I wonder, whether we should really drop the *Component abstract classes
>> (RuntimeExtensionComponent and UseProviderComponent). I don’t think they
>> provide real value at all but have an activate method, which they expect to
>> be called and which they expect to be a DS component annotated with the
>> Felix annotations. I think this is brittle.
>>
>
> The abstract classes were written specifically for that activate method, in
> order to avoid writing those lines of code for every implementation.

We are working on removing these abstract classes by leveraging service properties for

 * UseProvider ranking (service.ranking)
 * ExtensionProvider naming

>
>
>
>> * UseProvider defines an ordering method and declares itself being
>> Comparable. I think this is wrong. The UseProviders should be sorted using
>> regular OSGi service ranking.
>>
>
> If we make the service.ranking property configurable (metatype = true) I
> guess we could use the OSGi ranking. Theoretically the Use providers can be
> configured to be called in a pre-determined order, depending on how the
> Sightly projects from an instance make use of the Use-API (Java Use-API
> objects deployed in bundles / Java Use objects near the component script in
> the repo / JS Use-API, etc.). Having the Use providers called in the most
> favourable way can increase performance for Sightly applications.

Yes, the service.ranking property can be added to metatype for configurable ranking.

>
>
>> * I wonder, whether we really have to have all this API exported at all:
>> Some API I don’t see implemented at all (e.g. BaseRenderUnit — or is that
>> the base class for the Java classes generated from the Sightly templates ?)
>>
>
> The BaseRenderUnit is indeed the base class for classes generated from
> Sightly scripts, which is why we need to export the other classes as well.
> In this regard Sightly is no different from the JSP engine.

Ok. We’d have to test this more. But I suspect we don’t really need to export the BaseRenderUnit abstract class.

IIRC the reason for exporting the base JSP servlet is that other classes in the same package must be exported.

>
>
>> * Last but not least (actually most important of all): There is close to
>> no JavaDoc. Before cutting a release of this bundle, JavaDoc must be
>> provided for the API. For example I am not sure, what the Record or the
>> ObjectModel interfaces are really used for.
>>
>
> It's on my TODO list.

Cool. Thanks.

Regards
Felix
Reply | Threaded
Open this post in threaded view
|

Re: [Sightly] API vs. Implementation

Radu Cotescu-3
Hi Felix,

I pushed all the required changes to the forked repo. Things left to do:

* check what we actually need to export for preventing class loading
problems
* expand JavaDoc

At this point I think that we can create a patch with our changes and apply
it to trunk.

Cheers,
Radu

On Thu, Nov 20, 2014 at 6:53 PM, Felix Meschberger <[hidden email]>
wrote:

> Hi
>
> Thanks for your reply.
>
> For those interested in following, Radu and I are cooperating on these
> changes in my GitHub fork at https://github.com/fmeschbe/sling
>
>
> > Am 19.11.2014 um 22:53 schrieb Radu Cotescu <[hidden email]>:
> >
> > Hi Felix,
> >
> > The API reorganisation looks good to me.
> >
> > On Wed, Nov 19, 2014 at 10:49 PM, Felix Meschberger <[hidden email]>
> > wrote:
> >
> >> Apart from splitting the base .api package, this are further
> discussions:
> >>
> >> * Exceptions are forming a hierarchy with SightlyEngineException being
> the
> >> root and itself extending from SlingException
> >>
> >
> > Sure, it makes sense.
>
> Done in the fork. The „root“ exception for Sightly is not SightlyException
> (SightlyEngineException renamed to SightlyException).
>
> >
> >
> >> * I wonder, whether we should really drop the *Component abstract
> classes
> >> (RuntimeExtensionComponent and UseProviderComponent). I don’t think they
> >> provide real value at all but have an activate method, which they
> expect to
> >> be called and which they expect to be a DS component annotated with the
> >> Felix annotations. I think this is brittle.
> >>
> >
> > The abstract classes were written specifically for that activate method,
> in
> > order to avoid writing those lines of code for every implementation.
>
> We are working on removing these abstract classes by leveraging service
> properties for
>
>  * UseProvider ranking (service.ranking)
>  * ExtensionProvider naming
>
> >
> >
> >
> >> * UseProvider defines an ordering method and declares itself being
> >> Comparable. I think this is wrong. The UseProviders should be sorted
> using
> >> regular OSGi service ranking.
> >>
> >
> > If we make the service.ranking property configurable (metatype = true) I
> > guess we could use the OSGi ranking. Theoretically the Use providers can
> be
> > configured to be called in a pre-determined order, depending on how the
> > Sightly projects from an instance make use of the Use-API (Java Use-API
> > objects deployed in bundles / Java Use objects near the component script
> in
> > the repo / JS Use-API, etc.). Having the Use providers called in the most
> > favourable way can increase performance for Sightly applications.
>
> Yes, the service.ranking property can be added to metatype for
> configurable ranking.
>
> >
> >
> >> * I wonder, whether we really have to have all this API exported at all:
> >> Some API I don’t see implemented at all (e.g. BaseRenderUnit — or is
> that
> >> the base class for the Java classes generated from the Sightly
> templates ?)
> >>
> >
> > The BaseRenderUnit is indeed the base class for classes generated from
> > Sightly scripts, which is why we need to export the other classes as
> well.
> > In this regard Sightly is no different from the JSP engine.
>
> Ok. We’d have to test this more. But I suspect we don’t really need to
> export the BaseRenderUnit abstract class.
>
> IIRC the reason for exporting the base JSP servlet is that other classes
> in the same package must be exported.
>
> >
> >
> >> * Last but not least (actually most important of all): There is close to
> >> no JavaDoc. Before cutting a release of this bundle, JavaDoc must be
> >> provided for the API. For example I am not sure, what the Record or the
> >> ObjectModel interfaces are really used for.
> >>
> >
> > It's on my TODO list.
>
> Cool. Thanks.
>
> Regards
> Felix
>
Reply | Threaded
Open this post in threaded view
|

Re: [Sightly] API vs. Implementation

Felix Meschberger-3
Hi Radu

> Am 21.11.2014 um 18:39 schrieb Radu Cotescu <[hidden email]>:
>
> Hi Felix,
>
> I pushed all the required changes to the forked repo. Things left to do:
>
> * check what we actually need to export for preventing class loading
> problems

Excellent. I would prefer to really not export most of the RenderUnit stuff incl. BaseRenderUnit and RenderContext.

At one point I had a change, where I actually had RenderContext as an interface with a subset of the API, basically just getWriter (does that need to be a StackedWriter after all ?) and a RenderContextImpl which had all the rest but was internal.


> * expand JavaDoc
>
> At this point I think that we can create a patch with our changes and apply
> it to trunk.

We might want to continue on the fork to find the classes that we really want/need to export. Because this may cause further shuffling of packages etc.


Thanks
Felix

>
> Cheers,
> Radu
>
> On Thu, Nov 20, 2014 at 6:53 PM, Felix Meschberger <[hidden email]>
> wrote:
>
>> Hi
>>
>> Thanks for your reply.
>>
>> For those interested in following, Radu and I are cooperating on these
>> changes in my GitHub fork at https://github.com/fmeschbe/sling
>>
>>
>>> Am 19.11.2014 um 22:53 schrieb Radu Cotescu <[hidden email]>:
>>>
>>> Hi Felix,
>>>
>>> The API reorganisation looks good to me.
>>>
>>> On Wed, Nov 19, 2014 at 10:49 PM, Felix Meschberger <[hidden email]>
>>> wrote:
>>>
>>>> Apart from splitting the base .api package, this are further
>> discussions:
>>>>
>>>> * Exceptions are forming a hierarchy with SightlyEngineException being
>> the
>>>> root and itself extending from SlingException
>>>>
>>>
>>> Sure, it makes sense.
>>
>> Done in the fork. The „root“ exception for Sightly is not SightlyException
>> (SightlyEngineException renamed to SightlyException).
>>
>>>
>>>
>>>> * I wonder, whether we should really drop the *Component abstract
>> classes
>>>> (RuntimeExtensionComponent and UseProviderComponent). I don’t think they
>>>> provide real value at all but have an activate method, which they
>> expect to
>>>> be called and which they expect to be a DS component annotated with the
>>>> Felix annotations. I think this is brittle.
>>>>
>>>
>>> The abstract classes were written specifically for that activate method,
>> in
>>> order to avoid writing those lines of code for every implementation.
>>
>> We are working on removing these abstract classes by leveraging service
>> properties for
>>
>> * UseProvider ranking (service.ranking)
>> * ExtensionProvider naming
>>
>>>
>>>
>>>
>>>> * UseProvider defines an ordering method and declares itself being
>>>> Comparable. I think this is wrong. The UseProviders should be sorted
>> using
>>>> regular OSGi service ranking.
>>>>
>>>
>>> If we make the service.ranking property configurable (metatype = true) I
>>> guess we could use the OSGi ranking. Theoretically the Use providers can
>> be
>>> configured to be called in a pre-determined order, depending on how the
>>> Sightly projects from an instance make use of the Use-API (Java Use-API
>>> objects deployed in bundles / Java Use objects near the component script
>> in
>>> the repo / JS Use-API, etc.). Having the Use providers called in the most
>>> favourable way can increase performance for Sightly applications.
>>
>> Yes, the service.ranking property can be added to metatype for
>> configurable ranking.
>>
>>>
>>>
>>>> * I wonder, whether we really have to have all this API exported at all:
>>>> Some API I don’t see implemented at all (e.g. BaseRenderUnit — or is
>> that
>>>> the base class for the Java classes generated from the Sightly
>> templates ?)
>>>>
>>>
>>> The BaseRenderUnit is indeed the base class for classes generated from
>>> Sightly scripts, which is why we need to export the other classes as
>> well.
>>> In this regard Sightly is no different from the JSP engine.
>>
>> Ok. We’d have to test this more. But I suspect we don’t really need to
>> export the BaseRenderUnit abstract class.
>>
>> IIRC the reason for exporting the base JSP servlet is that other classes
>> in the same package must be exported.
>>
>>>
>>>
>>>> * Last but not least (actually most important of all): There is close to
>>>> no JavaDoc. Before cutting a release of this bundle, JavaDoc must be
>>>> provided for the API. For example I am not sure, what the Record or the
>>>> ObjectModel interfaces are really used for.
>>>>
>>>
>>> It's on my TODO list.
>>
>> Cool. Thanks.
>>
>> Regards
>> Felix
>>

Reply | Threaded
Open this post in threaded view
|

Re: [Sightly] API vs. Implementation

Radu Cotescu-3
Hi Felix,

I've pushed some new changes: BaseRenderUnit is now a part of impl.
RenderContext should indeed become an interface. Also, I think that we can
get rid of the UnitLocator interface, therefore reducing the size of the
exported API to the bare minimum.

Thanks,
Radu

On Mon, Nov 24, 2014 at 12:36 PM, Felix Meschberger <[hidden email]>
wrote:

> Hi Radu
>
> > Am 21.11.2014 um 18:39 schrieb Radu Cotescu <[hidden email]>:
> >
> > Hi Felix,
> >
> > I pushed all the required changes to the forked repo. Things left to do:
> >
> > * check what we actually need to export for preventing class loading
> > problems
>
> Excellent. I would prefer to really not export most of the RenderUnit
> stuff incl. BaseRenderUnit and RenderContext.
>
> At one point I had a change, where I actually had RenderContext as an
> interface with a subset of the API, basically just getWriter (does that
> need to be a StackedWriter after all ?) and a RenderContextImpl which had
> all the rest but was internal.
>
>
> > * expand JavaDoc
> >
> > At this point I think that we can create a patch with our changes and
> apply
> > it to trunk.
>
> We might want to continue on the fork to find the classes that we really
> want/need to export. Because this may cause further shuffling of packages
> etc.
>
>
> Thanks
> Felix
>
> >
> > Cheers,
> > Radu
> >
> > On Thu, Nov 20, 2014 at 6:53 PM, Felix Meschberger <[hidden email]>
> > wrote:
> >
> >> Hi
> >>
> >> Thanks for your reply.
> >>
> >> For those interested in following, Radu and I are cooperating on these
> >> changes in my GitHub fork at https://github.com/fmeschbe/sling
> >>
> >>
> >>> Am 19.11.2014 um 22:53 schrieb Radu Cotescu <[hidden email]>:
> >>>
> >>> Hi Felix,
> >>>
> >>> The API reorganisation looks good to me.
> >>>
> >>> On Wed, Nov 19, 2014 at 10:49 PM, Felix Meschberger <
> [hidden email]>
> >>> wrote:
> >>>
> >>>> Apart from splitting the base .api package, this are further
> >> discussions:
> >>>>
> >>>> * Exceptions are forming a hierarchy with SightlyEngineException being
> >> the
> >>>> root and itself extending from SlingException
> >>>>
> >>>
> >>> Sure, it makes sense.
> >>
> >> Done in the fork. The „root“ exception for Sightly is not
> SightlyException
> >> (SightlyEngineException renamed to SightlyException).
> >>
> >>>
> >>>
> >>>> * I wonder, whether we should really drop the *Component abstract
> >> classes
> >>>> (RuntimeExtensionComponent and UseProviderComponent). I don’t think
> they
> >>>> provide real value at all but have an activate method, which they
> >> expect to
> >>>> be called and which they expect to be a DS component annotated with
> the
> >>>> Felix annotations. I think this is brittle.
> >>>>
> >>>
> >>> The abstract classes were written specifically for that activate
> method,
> >> in
> >>> order to avoid writing those lines of code for every implementation.
> >>
> >> We are working on removing these abstract classes by leveraging service
> >> properties for
> >>
> >> * UseProvider ranking (service.ranking)
> >> * ExtensionProvider naming
> >>
> >>>
> >>>
> >>>
> >>>> * UseProvider defines an ordering method and declares itself being
> >>>> Comparable. I think this is wrong. The UseProviders should be sorted
> >> using
> >>>> regular OSGi service ranking.
> >>>>
> >>>
> >>> If we make the service.ranking property configurable (metatype = true)
> I
> >>> guess we could use the OSGi ranking. Theoretically the Use providers
> can
> >> be
> >>> configured to be called in a pre-determined order, depending on how the
> >>> Sightly projects from an instance make use of the Use-API (Java Use-API
> >>> objects deployed in bundles / Java Use objects near the component
> script
> >> in
> >>> the repo / JS Use-API, etc.). Having the Use providers called in the
> most
> >>> favourable way can increase performance for Sightly applications.
> >>
> >> Yes, the service.ranking property can be added to metatype for
> >> configurable ranking.
> >>
> >>>
> >>>
> >>>> * I wonder, whether we really have to have all this API exported at
> all:
> >>>> Some API I don’t see implemented at all (e.g. BaseRenderUnit — or is
> >> that
> >>>> the base class for the Java classes generated from the Sightly
> >> templates ?)
> >>>>
> >>>
> >>> The BaseRenderUnit is indeed the base class for classes generated from
> >>> Sightly scripts, which is why we need to export the other classes as
> >> well.
> >>> In this regard Sightly is no different from the JSP engine.
> >>
> >> Ok. We’d have to test this more. But I suspect we don’t really need to
> >> export the BaseRenderUnit abstract class.
> >>
> >> IIRC the reason for exporting the base JSP servlet is that other classes
> >> in the same package must be exported.
> >>
> >>>
> >>>
> >>>> * Last but not least (actually most important of all): There is close
> to
> >>>> no JavaDoc. Before cutting a release of this bundle, JavaDoc must be
> >>>> provided for the API. For example I am not sure, what the Record or
> the
> >>>> ObjectModel interfaces are really used for.
> >>>>
> >>>
> >>> It's on my TODO list.
> >>
> >> Cool. Thanks.
> >>
> >> Regards
> >> Felix
> >>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [Sightly] API vs. Implementation

Radu Cotescu-3
Hi Felix,

The API refactoring should now be completed - I've refactored the
RenderContext as well and there's no UnitLocator anymore (its usage was
restricted to only the RenderUnitProvider which can "locate" units by
itself).

We can review the changes before deciding to merge them to Sling trunk if
you think that we might improve some other areas.

Regards,
Radu



On Tue, Nov 25, 2014 at 12:05 AM, Radu Cotescu <[hidden email]> wrote:

> Hi Felix,
>
> I've pushed some new changes: BaseRenderUnit is now a part of impl.
> RenderContext should indeed become an interface. Also, I think that we can
> get rid of the UnitLocator interface, therefore reducing the size of the
> exported API to the bare minimum.
>
> Thanks,
> Radu
>
> On Mon, Nov 24, 2014 at 12:36 PM, Felix Meschberger <[hidden email]>
> wrote:
>
>> Hi Radu
>>
>> > Am 21.11.2014 um 18:39 schrieb Radu Cotescu <[hidden email]>:
>> >
>> > Hi Felix,
>> >
>> > I pushed all the required changes to the forked repo. Things left to do:
>> >
>> > * check what we actually need to export for preventing class loading
>> > problems
>>
>> Excellent. I would prefer to really not export most of the RenderUnit
>> stuff incl. BaseRenderUnit and RenderContext.
>>
>> At one point I had a change, where I actually had RenderContext as an
>> interface with a subset of the API, basically just getWriter (does that
>> need to be a StackedWriter after all ?) and a RenderContextImpl which had
>> all the rest but was internal.
>>
>>
>> > * expand JavaDoc
>> >
>> > At this point I think that we can create a patch with our changes and
>> apply
>> > it to trunk.
>>
>> We might want to continue on the fork to find the classes that we really
>> want/need to export. Because this may cause further shuffling of packages
>> etc.
>>
>>
>> Thanks
>> Felix
>>
>> >
>> > Cheers,
>> > Radu
>> >
>> > On Thu, Nov 20, 2014 at 6:53 PM, Felix Meschberger <[hidden email]>
>> > wrote:
>> >
>> >> Hi
>> >>
>> >> Thanks for your reply.
>> >>
>> >> For those interested in following, Radu and I are cooperating on these
>> >> changes in my GitHub fork at https://github.com/fmeschbe/sling
>> >>
>> >>
>> >>> Am 19.11.2014 um 22:53 schrieb Radu Cotescu <[hidden email]>:
>> >>>
>> >>> Hi Felix,
>> >>>
>> >>> The API reorganisation looks good to me.
>> >>>
>> >>> On Wed, Nov 19, 2014 at 10:49 PM, Felix Meschberger <
>> [hidden email]>
>> >>> wrote:
>> >>>
>> >>>> Apart from splitting the base .api package, this are further
>> >> discussions:
>> >>>>
>> >>>> * Exceptions are forming a hierarchy with SightlyEngineException
>> being
>> >> the
>> >>>> root and itself extending from SlingException
>> >>>>
>> >>>
>> >>> Sure, it makes sense.
>> >>
>> >> Done in the fork. The „root“ exception for Sightly is not
>> SightlyException
>> >> (SightlyEngineException renamed to SightlyException).
>> >>
>> >>>
>> >>>
>> >>>> * I wonder, whether we should really drop the *Component abstract
>> >> classes
>> >>>> (RuntimeExtensionComponent and UseProviderComponent). I don’t think
>> they
>> >>>> provide real value at all but have an activate method, which they
>> >> expect to
>> >>>> be called and which they expect to be a DS component annotated with
>> the
>> >>>> Felix annotations. I think this is brittle.
>> >>>>
>> >>>
>> >>> The abstract classes were written specifically for that activate
>> method,
>> >> in
>> >>> order to avoid writing those lines of code for every implementation.
>> >>
>> >> We are working on removing these abstract classes by leveraging service
>> >> properties for
>> >>
>> >> * UseProvider ranking (service.ranking)
>> >> * ExtensionProvider naming
>> >>
>> >>>
>> >>>
>> >>>
>> >>>> * UseProvider defines an ordering method and declares itself being
>> >>>> Comparable. I think this is wrong. The UseProviders should be sorted
>> >> using
>> >>>> regular OSGi service ranking.
>> >>>>
>> >>>
>> >>> If we make the service.ranking property configurable (metatype =
>> true) I
>> >>> guess we could use the OSGi ranking. Theoretically the Use providers
>> can
>> >> be
>> >>> configured to be called in a pre-determined order, depending on how
>> the
>> >>> Sightly projects from an instance make use of the Use-API (Java
>> Use-API
>> >>> objects deployed in bundles / Java Use objects near the component
>> script
>> >> in
>> >>> the repo / JS Use-API, etc.). Having the Use providers called in the
>> most
>> >>> favourable way can increase performance for Sightly applications.
>> >>
>> >> Yes, the service.ranking property can be added to metatype for
>> >> configurable ranking.
>> >>
>> >>>
>> >>>
>> >>>> * I wonder, whether we really have to have all this API exported at
>> all:
>> >>>> Some API I don’t see implemented at all (e.g. BaseRenderUnit — or is
>> >> that
>> >>>> the base class for the Java classes generated from the Sightly
>> >> templates ?)
>> >>>>
>> >>>
>> >>> The BaseRenderUnit is indeed the base class for classes generated from
>> >>> Sightly scripts, which is why we need to export the other classes as
>> >> well.
>> >>> In this regard Sightly is no different from the JSP engine.
>> >>
>> >> Ok. We’d have to test this more. But I suspect we don’t really need to
>> >> export the BaseRenderUnit abstract class.
>> >>
>> >> IIRC the reason for exporting the base JSP servlet is that other
>> classes
>> >> in the same package must be exported.
>> >>
>> >>>
>> >>>
>> >>>> * Last but not least (actually most important of all): There is
>> close to
>> >>>> no JavaDoc. Before cutting a release of this bundle, JavaDoc must be
>> >>>> provided for the API. For example I am not sure, what the Record or
>> the
>> >>>> ObjectModel interfaces are really used for.
>> >>>>
>> >>>
>> >>> It's on my TODO list.
>> >>
>> >> Cool. Thanks.
>> >>
>> >> Regards
>> >> Felix
>> >>
>>
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: [Sightly] API vs. Implementation

Felix Meschberger-3
Hi Radu

Cool, thanks. I'll review and get back to you.

Regards
Felix

--
Typos caused by my iPhone

> Am 25.11.2014 um 13:39 schrieb Radu Cotescu <[hidden email]>:
>
> Hi Felix,
>
> The API refactoring should now be completed - I've refactored the
> RenderContext as well and there's no UnitLocator anymore (its usage was
> restricted to only the RenderUnitProvider which can "locate" units by
> itself).
>
> We can review the changes before deciding to merge them to Sling trunk if
> you think that we might improve some other areas.
>
> Regards,
> Radu
>
>
>
>> On Tue, Nov 25, 2014 at 12:05 AM, Radu Cotescu <[hidden email]> wrote:
>>
>> Hi Felix,
>>
>> I've pushed some new changes: BaseRenderUnit is now a part of impl.
>> RenderContext should indeed become an interface. Also, I think that we can
>> get rid of the UnitLocator interface, therefore reducing the size of the
>> exported API to the bare minimum.
>>
>> Thanks,
>> Radu
>>
>> On Mon, Nov 24, 2014 at 12:36 PM, Felix Meschberger <[hidden email]>
>> wrote:
>>
>>> Hi Radu
>>>
>>>> Am 21.11.2014 um 18:39 schrieb Radu Cotescu <[hidden email]>:
>>>>
>>>> Hi Felix,
>>>>
>>>> I pushed all the required changes to the forked repo. Things left to do:
>>>>
>>>> * check what we actually need to export for preventing class loading
>>>> problems
>>>
>>> Excellent. I would prefer to really not export most of the RenderUnit
>>> stuff incl. BaseRenderUnit and RenderContext.
>>>
>>> At one point I had a change, where I actually had RenderContext as an
>>> interface with a subset of the API, basically just getWriter (does that
>>> need to be a StackedWriter after all ?) and a RenderContextImpl which had
>>> all the rest but was internal.
>>>
>>>
>>>> * expand JavaDoc
>>>>
>>>> At this point I think that we can create a patch with our changes and
>>> apply
>>>> it to trunk.
>>>
>>> We might want to continue on the fork to find the classes that we really
>>> want/need to export. Because this may cause further shuffling of packages
>>> etc.
>>>
>>>
>>> Thanks
>>> Felix
>>>
>>>>
>>>> Cheers,
>>>> Radu
>>>>
>>>> On Thu, Nov 20, 2014 at 6:53 PM, Felix Meschberger <[hidden email]>
>>>> wrote:
>>>>
>>>>> Hi
>>>>>
>>>>> Thanks for your reply.
>>>>>
>>>>> For those interested in following, Radu and I are cooperating on these
>>>>> changes in my GitHub fork at https://github.com/fmeschbe/sling
>>>>>
>>>>>
>>>>>> Am 19.11.2014 um 22:53 schrieb Radu Cotescu <[hidden email]>:
>>>>>>
>>>>>> Hi Felix,
>>>>>>
>>>>>> The API reorganisation looks good to me.
>>>>>>
>>>>>> On Wed, Nov 19, 2014 at 10:49 PM, Felix Meschberger <
>>> [hidden email]>
>>>>>> wrote:
>>>>>>
>>>>>>> Apart from splitting the base .api package, this are further
>>>>> discussions:
>>>>>>>
>>>>>>> * Exceptions are forming a hierarchy with SightlyEngineException
>>> being
>>>>> the
>>>>>>> root and itself extending from SlingException
>>>>>>>
>>>>>>
>>>>>> Sure, it makes sense.
>>>>>
>>>>> Done in the fork. The „root“ exception for Sightly is not
>>> SightlyException
>>>>> (SightlyEngineException renamed to SightlyException).
>>>>>
>>>>>>
>>>>>>
>>>>>>> * I wonder, whether we should really drop the *Component abstract
>>>>> classes
>>>>>>> (RuntimeExtensionComponent and UseProviderComponent). I don’t think
>>> they
>>>>>>> provide real value at all but have an activate method, which they
>>>>> expect to
>>>>>>> be called and which they expect to be a DS component annotated with
>>> the
>>>>>>> Felix annotations. I think this is brittle.
>>>>>>>
>>>>>>
>>>>>> The abstract classes were written specifically for that activate
>>> method,
>>>>> in
>>>>>> order to avoid writing those lines of code for every implementation.
>>>>>
>>>>> We are working on removing these abstract classes by leveraging service
>>>>> properties for
>>>>>
>>>>> * UseProvider ranking (service.ranking)
>>>>> * ExtensionProvider naming
>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>> * UseProvider defines an ordering method and declares itself being
>>>>>>> Comparable. I think this is wrong. The UseProviders should be sorted
>>>>> using
>>>>>>> regular OSGi service ranking.
>>>>>>>
>>>>>>
>>>>>> If we make the service.ranking property configurable (metatype =
>>> true) I
>>>>>> guess we could use the OSGi ranking. Theoretically the Use providers
>>> can
>>>>> be
>>>>>> configured to be called in a pre-determined order, depending on how
>>> the
>>>>>> Sightly projects from an instance make use of the Use-API (Java
>>> Use-API
>>>>>> objects deployed in bundles / Java Use objects near the component
>>> script
>>>>> in
>>>>>> the repo / JS Use-API, etc.). Having the Use providers called in the
>>> most
>>>>>> favourable way can increase performance for Sightly applications.
>>>>>
>>>>> Yes, the service.ranking property can be added to metatype for
>>>>> configurable ranking.
>>>>>
>>>>>>
>>>>>>
>>>>>>> * I wonder, whether we really have to have all this API exported at
>>> all:
>>>>>>> Some API I don’t see implemented at all (e.g. BaseRenderUnit — or is
>>>>> that
>>>>>>> the base class for the Java classes generated from the Sightly
>>>>> templates ?)
>>>>>>>
>>>>>>
>>>>>> The BaseRenderUnit is indeed the base class for classes generated from
>>>>>> Sightly scripts, which is why we need to export the other classes as
>>>>> well.
>>>>>> In this regard Sightly is no different from the JSP engine.
>>>>>
>>>>> Ok. We’d have to test this more. But I suspect we don’t really need to
>>>>> export the BaseRenderUnit abstract class.
>>>>>
>>>>> IIRC the reason for exporting the base JSP servlet is that other
>>> classes
>>>>> in the same package must be exported.
>>>>>
>>>>>>
>>>>>>
>>>>>>> * Last but not least (actually most important of all): There is
>>> close to
>>>>>>> no JavaDoc. Before cutting a release of this bundle, JavaDoc must be
>>>>>>> provided for the API. For example I am not sure, what the Record or
>>> the
>>>>>>> ObjectModel interfaces are really used for.
>>>>>>>
>>>>>>
>>>>>> It's on my TODO list.
>>>>>
>>>>> Cool. Thanks.
>>>>>
>>>>> Regards
>>>>> Felix
>>>>>
>>>
>>>
>>
Reply | Threaded
Open this post in threaded view
|

Re: [Sightly] API vs. Implementation

Felix Meschberger-3
Hi all

Radu and I have completed the refactoring of the Sightly Engine API in our fork at [1] and I am currently committing the large patch resulting from that refactoring for SLING-4206 [2]. The major part of this patch is about moving classes into „impl“ packages.

So, there will be a number of commit messages coming along …

Regards
Felix

[1] https://github.com/fmeschbe/sling.git
[2] https://issues.apache.org/jira/browse/SLING-4206

> Am 25.11.2014 um 15:46 schrieb Felix Meschberger <[hidden email]>:
>
> Hi Radu
>
> Cool, thanks. I'll review and get back to you.
>
> Regards
> Felix
>
> --
> Typos caused by my iPhone
>
>> Am 25.11.2014 um 13:39 schrieb Radu Cotescu <[hidden email]>:
>>
>> Hi Felix,
>>
>> The API refactoring should now be completed - I've refactored the
>> RenderContext as well and there's no UnitLocator anymore (its usage was
>> restricted to only the RenderUnitProvider which can "locate" units by
>> itself).
>>
>> We can review the changes before deciding to merge them to Sling trunk if
>> you think that we might improve some other areas.
>>
>> Regards,
>> Radu
>>
>>
>>
>>> On Tue, Nov 25, 2014 at 12:05 AM, Radu Cotescu <[hidden email]> wrote:
>>>
>>> Hi Felix,
>>>
>>> I've pushed some new changes: BaseRenderUnit is now a part of impl.
>>> RenderContext should indeed become an interface. Also, I think that we can
>>> get rid of the UnitLocator interface, therefore reducing the size of the
>>> exported API to the bare minimum.
>>>
>>> Thanks,
>>> Radu
>>>
>>> On Mon, Nov 24, 2014 at 12:36 PM, Felix Meschberger <[hidden email]>
>>> wrote:
>>>
>>>> Hi Radu
>>>>
>>>>> Am 21.11.2014 um 18:39 schrieb Radu Cotescu <[hidden email]>:
>>>>>
>>>>> Hi Felix,
>>>>>
>>>>> I pushed all the required changes to the forked repo. Things left to do:
>>>>>
>>>>> * check what we actually need to export for preventing class loading
>>>>> problems
>>>>
>>>> Excellent. I would prefer to really not export most of the RenderUnit
>>>> stuff incl. BaseRenderUnit and RenderContext.
>>>>
>>>> At one point I had a change, where I actually had RenderContext as an
>>>> interface with a subset of the API, basically just getWriter (does that
>>>> need to be a StackedWriter after all ?) and a RenderContextImpl which had
>>>> all the rest but was internal.
>>>>
>>>>
>>>>> * expand JavaDoc
>>>>>
>>>>> At this point I think that we can create a patch with our changes and
>>>> apply
>>>>> it to trunk.
>>>>
>>>> We might want to continue on the fork to find the classes that we really
>>>> want/need to export. Because this may cause further shuffling of packages
>>>> etc.
>>>>
>>>>
>>>> Thanks
>>>> Felix
>>>>
>>>>>
>>>>> Cheers,
>>>>> Radu
>>>>>
>>>>> On Thu, Nov 20, 2014 at 6:53 PM, Felix Meschberger <[hidden email]>
>>>>> wrote:
>>>>>
>>>>>> Hi
>>>>>>
>>>>>> Thanks for your reply.
>>>>>>
>>>>>> For those interested in following, Radu and I are cooperating on these
>>>>>> changes in my GitHub fork at https://github.com/fmeschbe/sling
>>>>>>
>>>>>>
>>>>>>> Am 19.11.2014 um 22:53 schrieb Radu Cotescu <[hidden email]>:
>>>>>>>
>>>>>>> Hi Felix,
>>>>>>>
>>>>>>> The API reorganisation looks good to me.
>>>>>>>
>>>>>>> On Wed, Nov 19, 2014 at 10:49 PM, Felix Meschberger <
>>>> [hidden email]>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> Apart from splitting the base .api package, this are further
>>>>>> discussions:
>>>>>>>>
>>>>>>>> * Exceptions are forming a hierarchy with SightlyEngineException
>>>> being
>>>>>> the
>>>>>>>> root and itself extending from SlingException
>>>>>>>>
>>>>>>>
>>>>>>> Sure, it makes sense.
>>>>>>
>>>>>> Done in the fork. The „root“ exception for Sightly is not
>>>> SightlyException
>>>>>> (SightlyEngineException renamed to SightlyException).
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> * I wonder, whether we should really drop the *Component abstract
>>>>>> classes
>>>>>>>> (RuntimeExtensionComponent and UseProviderComponent). I don’t think
>>>> they
>>>>>>>> provide real value at all but have an activate method, which they
>>>>>> expect to
>>>>>>>> be called and which they expect to be a DS component annotated with
>>>> the
>>>>>>>> Felix annotations. I think this is brittle.
>>>>>>>>
>>>>>>>
>>>>>>> The abstract classes were written specifically for that activate
>>>> method,
>>>>>> in
>>>>>>> order to avoid writing those lines of code for every implementation.
>>>>>>
>>>>>> We are working on removing these abstract classes by leveraging service
>>>>>> properties for
>>>>>>
>>>>>> * UseProvider ranking (service.ranking)
>>>>>> * ExtensionProvider naming
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> * UseProvider defines an ordering method and declares itself being
>>>>>>>> Comparable. I think this is wrong. The UseProviders should be sorted
>>>>>> using
>>>>>>>> regular OSGi service ranking.
>>>>>>>>
>>>>>>>
>>>>>>> If we make the service.ranking property configurable (metatype =
>>>> true) I
>>>>>>> guess we could use the OSGi ranking. Theoretically the Use providers
>>>> can
>>>>>> be
>>>>>>> configured to be called in a pre-determined order, depending on how
>>>> the
>>>>>>> Sightly projects from an instance make use of the Use-API (Java
>>>> Use-API
>>>>>>> objects deployed in bundles / Java Use objects near the component
>>>> script
>>>>>> in
>>>>>>> the repo / JS Use-API, etc.). Having the Use providers called in the
>>>> most
>>>>>>> favourable way can increase performance for Sightly applications.
>>>>>>
>>>>>> Yes, the service.ranking property can be added to metatype for
>>>>>> configurable ranking.
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> * I wonder, whether we really have to have all this API exported at
>>>> all:
>>>>>>>> Some API I don’t see implemented at all (e.g. BaseRenderUnit — or is
>>>>>> that
>>>>>>>> the base class for the Java classes generated from the Sightly
>>>>>> templates ?)
>>>>>>>>
>>>>>>>
>>>>>>> The BaseRenderUnit is indeed the base class for classes generated from
>>>>>>> Sightly scripts, which is why we need to export the other classes as
>>>>>> well.
>>>>>>> In this regard Sightly is no different from the JSP engine.
>>>>>>
>>>>>> Ok. We’d have to test this more. But I suspect we don’t really need to
>>>>>> export the BaseRenderUnit abstract class.
>>>>>>
>>>>>> IIRC the reason for exporting the base JSP servlet is that other
>>>> classes
>>>>>> in the same package must be exported.
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> * Last but not least (actually most important of all): There is
>>>> close to
>>>>>>>> no JavaDoc. Before cutting a release of this bundle, JavaDoc must be
>>>>>>>> provided for the API. For example I am not sure, what the Record or
>>>> the
>>>>>>>> ObjectModel interfaces are really used for.
>>>>>>>>
>>>>>>>
>>>>>>> It's on my TODO list.
>>>>>>
>>>>>> Cool. Thanks.
>>>>>>
>>>>>> Regards
>>>>>> Felix
>>>>>>
>>>>
>>>>
>>>