On Thu, May 13, 2010 at 12:01 PM, Paul Lindner <[email protected]>wrote:
> hrm..
>
> Due to the ordered magic of ImmutableSet we do get things injected in the
> correct order:
>
I sort of figured that to be the case, though I'm still a bit loath for
important behavior to depend on quasi-magic (Guice :)) combined with what's
arguably an accident of implementation details rather than a prescribed API.
>
> As you can see below we inject the random ordered multibindings first,
> followed by the comma split paths of "shindig.features.default".
>
> @Provides
> @Singleton
> @Named("org.apache.shindig.features")
> protected Set<String>
> defaultFeatures(@Named("shindig.features.default")String features,
>
> @Named("org.apache.shindig.features-extended")Set<String> extended) {
> return
>
> ImmutableSet.<String>builder().addAll(extended).add(StringUtils.split(features,
> ',')).build();
> }
>
Yep, I did take close notice of this -- that looks like the right logic to
me.
In our particular case, we'd be reasonably well-served by simply @Inject'ing
a List<String> into FeatureRegistry constructed via the two Sets
(String.split and Set<..> extended), guaranteeing at least relative order of
those two (and order of the features provided by the comma-delimited String.
Since this gives users flexibility to guarantee order when they _really_
need to do so, and to perform one-time extensions past that, this seems
generally reasonable, if not totally ideal.
>
> So I believe this is safe to apply as-is. Still it seems like there should
> be a better way to do this. The use cases I can see that we might need to
> address:
>
> * Override default features (works by insuring that we add user features
> last).
* Removing features from the default. Trickier... Currently you have to
> copy features.txt and remove the ones you don't want. Might make sense to
> have a blacklist of features you don't want to support.
>
The former case here seems pretty common; the latter isn't possible today,
so I'd bucket it as a "new feature" once it's demanded.
>
> There are a bunch of things we inject that are order-dependent (basically
> Lists) that would be nice to support in a plugin/contributor style. The
> only easy way to do this is to inject available features as a Set/Map and
> then apply some filter and/or ordering.
>
> Consider Rewriters for example. We might define an ordered 'phase' enum
> that each rewriter runs in and use a default comparator for that. However
> we can add specific rules that can insure that Rewriter B runs after
> Rewriter A by using a custom compare() method.
>
> More info on Set vs List here.
>
> http://publicobject.com/2008/05/why-no-multibindings-for-lists-or.html
Agreed that Set/Map injection via contributor style is a laudable goal, and
I appreciate you/we are moving toward it. But I'm a bit loath to invent some
kind of ordering-fu (Comparator for instance) as an especially-magical
implementation requirement for ordering... hmm...
--j
>
>
> On Thu, May 13, 2010 at 11:25 AM, John Hjelmstad <[email protected]> wrote:
>
> > This actually isn't backward-compatible. Set<String> is unordered,
> whereas
> > the previous ordered array makes it possible to override existing feature
> > declarations. For instance, we have a custom rpc/feature.xml override of
> > the
> > Shindig default.
> >
> > --j
> >
> > On Thu, May 13, 2010 at 6:58 AM, Paul Lindner <[email protected]> wrote:
> >
> > > Here's the next step towards allowing a shindig plugin architecture.
> > This
> > > one allows external modules to add their javascript features to the
> > > FeatureRegistry.
> > >
> > > It's backwards compatible with the way we currently inject feature
> > > directories (via shindig.features.default) so I'm going to commit this
> > soon
> > > unless there are objections.
> > >
> > > On Thu, May 13, 2010 at 6:53 AM, <[email protected]> wrote:
> > >
> > > > Reviewers: shindig.remailer_gmail.com,
> > > >
> > > >
> > > >
> > > > Please review this at http://codereview.appspot.com/1197041/show
> > > >
> > > > Affected files:
> > > > M
> > > >
> > >
> >
> java/gadgets/src/main/java/org/apache/shindig/gadgets/DefaultGuiceModule.java
> > > > M
> > > >
> > >
> >
> java/gadgets/src/main/java/org/apache/shindig/gadgets/features/FeatureRegistry.java
> > > > M
> > > >
> > >
> >
> java/gadgets/src/test/java/org/apache/shindig/gadgets/features/FeatureRegistryTest.java
> > > > M
> > > >
> > >
> >
> java/gadgets/src/test/java/org/apache/shindig/gadgets/templates/tags/TemplateBasedTagHandlerTest.java
> > > >
> > > >
> > > >
> > >
> >
>
