I guess it gets to philosophy then, notably that I remain nervous that Set "really" means OrderedSet, and always will. It's an implicit dependency on the particulars of the implementation rather than the contract of the interfaces involved.
I suspect this discussion could devolve into a referendum on using Set in Multibindings overall, but I'll assume the Guice folks have come up w/ a good reason for that. Which I hope isn't "it's a hedge, we can turn it into an unordered Set later" :) --j On Mon, Nov 8, 2010 at 2:52 PM, Paul Lindner <[email protected]> wrote: > The nice part about the patch is that MultiBinding results in an Ordered > Set > based on the order of the individual bind statements. > > So this means a third party module can only add a rewriter at the beginning > of the pipeline (by using a Guice Module that loads before the > RewriterModule) or by adding a rewriter at the end of the pipeline (by > adding a Guice Module that runs after the Rewrite Module) > > I suspect that one could also rejigger the order of the rewriters using > Modules.override(), but that remains to be seen. > > On Mon, Nov 8, 2010 at 2:41 PM, John Hjelmstad <[email protected]> wrote: > > > Hi Paul: > > > > Fundamental problem here is that rewriter ordering is important: a Set > > obviously doesn't offer that guarantee. > > > > I'd recommend we rejigger this implementation so that the Set-provided > > rewriters come as an addendum to those provided as "core" by Shindig. If > > relative ordering of *those* Rewriters is important, we can have pre-core > > and post-core Sets. > > > > Apologies for not having seen this in JIRA; my mailbox has gotten so > > overstuffed of late that some of the messages get lost in the aether. > > > > --j > > > > On Thu, Nov 4, 2010 at 3:58 PM, <[email protected]> wrote: > > > > > Author: lindner > > > Date: Thu Nov 4 22:58:54 2010 > > > New Revision: 1031332 > > > > > > URL: http://svn.apache.org/viewvc?rev=1031332&view=rev > > > Log: > > > SHINDIG-1456 | Patch from Kai Feng Zhang | Allow modules to add new > > > rewriters > > > > > > Modified: > > > > > > > > > > shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/GadgetRewritersProvider.java > > > > > > > > > > shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java > > > > > > > > > > shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/render/HtmlRendererTest.java > > > > > > Modified: > > > > > > shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/GadgetRewritersProvider.java > > > URL: > > > > > > http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/GadgetRewritersProvider.java?rev=1031332&r1=1031331&r2=1031332&view=diff > > > > > > > > > ============================================================================== > > > --- > > > > > > shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/GadgetRewritersProvider.java > > > (original) > > > +++ > > > > > > shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/GadgetRewritersProvider.java > > > Thu Nov 4 22:58:54 2010 > > > @@ -19,13 +19,13 @@ > > > > > > package org.apache.shindig.gadgets.render; > > > > > > -import com.google.inject.Inject; > > > -import com.google.inject.name.Named; > > > +import java.util.Set; > > > > > > import org.apache.shindig.gadgets.GadgetContext; > > > import org.apache.shindig.gadgets.rewrite.GadgetRewriter; > > > > > > -import java.util.List; > > > +import com.google.inject.Inject; > > > +import com.google.inject.name.Named; > > > > > > /** > > > * Class to provide list of rewriters according to gadget request. > > > @@ -34,15 +34,14 @@ import java.util.List; > > > * @since 2.0.0 > > > */ > > > public class GadgetRewritersProvider { > > > - private final List<GadgetRewriter> renderRewriters; > > > + private final Set<GadgetRewriter> renderRewriters; > > > > > > @Inject > > > - public GadgetRewritersProvider( > > > - @Named("shindig.rewriters.gadget") List<GadgetRewriter> > > > renderRewriters) { > > > + public GadgetRewritersProvider(@Named("shindig.rewriters.gadget") > > > Set<GadgetRewriter> renderRewriters) { > > > this.renderRewriters = renderRewriters; > > > } > > > > > > - public List<GadgetRewriter> getRewriters(GadgetContext context) { > > > + public Set<GadgetRewriter> getRewriters(GadgetContext context) { > > > return renderRewriters; > > > } > > > } > > > > > > Modified: > > > > > > shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java > > > URL: > > > > > > http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java?rev=1031332&r1=1031331&r2=1031332&view=diff > > > > > > > > > ============================================================================== > > > --- > > > > > > shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java > > > (original) > > > +++ > > > > > > shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java > > > Thu Nov 4 22:58:54 2010 > > > @@ -18,12 +18,8 @@ > > > */ > > > package org.apache.shindig.gadgets.rewrite; > > > > > > -import com.google.common.collect.ImmutableList; > > > -import com.google.inject.AbstractModule; > > > -import com.google.inject.Provides; > > > -import com.google.inject.Singleton; > > > -import com.google.inject.name.Named; > > > -import com.google.inject.name.Names; > > > +import java.util.List; > > > + > > > import org.apache.shindig.gadgets.parse.GadgetHtmlParser; > > > import org.apache.shindig.gadgets.render.CajaResponseRewriter; > > > import org.apache.shindig.gadgets.render.OpenSocialI18NGadgetRewriter; > > > @@ -33,7 +29,13 @@ import org.apache.shindig.gadgets.render > > > import org.apache.shindig.gadgets.rewrite.image.BasicImageRewriter; > > > import org.apache.shindig.gadgets.servlet.CajaContentRewriter; > > > > > > -import java.util.List; > > > +import com.google.common.collect.ImmutableList; > > > +import com.google.inject.AbstractModule; > > > +import com.google.inject.Provides; > > > +import com.google.inject.Singleton; > > > +import com.google.inject.multibindings.Multibinder; > > > +import com.google.inject.name.Named; > > > +import com.google.inject.name.Names; > > > > > > /** > > > * Guice bindings for the rewrite package. > > > @@ -45,25 +47,23 @@ public class RewriteModule extends Abstr > > > bind(ResponseRewriterRegistry.class) > > > > > > > > > .annotatedWith(Names.named("shindig.accelerate.response.rewriter.registry")) > > > .to(AccelResponseRewriterRegistry.class); > > > + > > > + configureRewriters(); > > > + > > > } > > > > > > - @Provides > > > - @Singleton > > > - @Named("shindig.rewriters.gadget") > > > - protected List<GadgetRewriter> provideGadgetRewriters( > > > - PipelineDataGadgetRewriter pipelineRewriter, > > > - TemplateRewriter templateRewriter, > > > - AbsolutePathReferenceRewriter absolutePathRewriter, > > > - StyleTagExtractorContentRewriter styleTagExtractorRewriter, > > > - StyleAdjacencyContentRewriter styleAdjacencyRewriter, > > > - ProxyingContentRewriter proxyingRewriter, > > > - CajaContentRewriter cajaRewriter, > > > - SanitizingGadgetRewriter sanitizedRewriter, > > > - RenderingGadgetRewriter renderingRewriter, > > > - OpenSocialI18NGadgetRewriter i18nRewriter) { > > > - return ImmutableList.of(pipelineRewriter, templateRewriter, > > > - absolutePathRewriter, styleTagExtractorRewriter, > > > styleAdjacencyRewriter, proxyingRewriter, > > > - cajaRewriter, sanitizedRewriter, renderingRewriter, > > i18nRewriter); > > > + private void configureRewriters() { > > > + Multibinder<GadgetRewriter> multibinder = > > > Multibinder.newSetBinder(binder(), GadgetRewriter.class, > > > Names.named("shindig.rewriters.gadget")); > > > + multibinder.addBinding().to(PipelineDataGadgetRewriter.class); > > > + multibinder.addBinding().to(TemplateRewriter.class); > > > + multibinder.addBinding().to(AbsolutePathReferenceRewriter.class); > > > + > multibinder.addBinding().to(StyleTagExtractorContentRewriter.class); > > > + multibinder.addBinding().to(StyleAdjacencyContentRewriter.class); > > > + multibinder.addBinding().to(ProxyingContentRewriter.class); > > > + multibinder.addBinding().to(CajaContentRewriter.class); > > > + multibinder.addBinding().to(SanitizingGadgetRewriter.class); > > > + multibinder.addBinding().to(RenderingGadgetRewriter.class); > > > + multibinder.addBinding().to(OpenSocialI18NGadgetRewriter.class); > > > } > > > > > > @Provides > > > > > > Modified: > > > > > > shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/render/HtmlRendererTest.java > > > URL: > > > > > > http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/render/HtmlRendererTest.java?rev=1031332&r1=1031331&r2=1031332&view=diff > > > > > > > > > ============================================================================== > > > --- > > > > > > shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/render/HtmlRendererTest.java > > > (original) > > > +++ > > > > > > shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/render/HtmlRendererTest.java > > > Thu Nov 4 22:58:54 2010 > > > @@ -42,6 +42,7 @@ import java.util.Collection; > > > import java.util.concurrent.Callable; > > > > > > import com.google.common.collect.ImmutableList; > > > +import com.google.common.collect.ImmutableSet; > > > > > > /** > > > * Tests for HtmlRenderer > > > @@ -84,7 +85,7 @@ public class HtmlRendererTest { > > > @Before > > > public void setUp() throws Exception { > > > renderer = new HtmlRenderer(preloaderService, proxyRenderer, > > > - new GadgetRewritersProvider(ImmutableList.of((GadgetRewriter) > > > captureRewriter)), > > > + new GadgetRewritersProvider(ImmutableSet.of((GadgetRewriter) > > > captureRewriter)), > > > null); > > > > > > } > > > > > > > > > > > > > > > -- > Paul Lindner -- [email protected] -- linkedin.com/in/plindner >
