I would like to add one thought to this reply, specifically around TCK
compliance.
The critical thing is that Opensocial spec compliance is paramount,
*not* TCK,
so commenting out existing tests is a blocker.
On this point, I disagree. As OpenSocial evolves, and is used in more
contexts, specifically, in enterprise environments, compliance with
standards becomes paramount. Part of the reason for this is because the
delivery model of the software is different--customers are installing
what you provide on site vs. using a hosted/cloud based service.
In many cases, what you deliver to customers MUST be compliant with the
TCK. This is a legal requirement, not a technical one.
That said, I like the idea of making it easy to plug in Jasper for JUEL
and believe this is a good thing to do. Obviously, my concern would be
for inconsistent behavior between sites because some may use Jasper and
other JUEL. I would recommend that we establish a default position of
being compliant with the TCK.
Also, I think we should tighten up the language in spec so there is no
ambiguities in the behavior so that going forward, we can have a spec
whose implementation can be TCK compliant.
On 2010/06/22 18:24:51, awiner1 wrote:
I like the faster performance - though I strongly recommend checking
out:
http://code.google.com/p/caliper/wiki/JavaMicrobenchmarks
... before you take results from a looping test as gospel (Caliper has
some
useful frameworks for gathering results). (Also, consider upgrading
to JUEL
2.2.1, which has fixes and may have performance improvements.)
The new dependency from DefaultTemplateProcessor to
ShindigTypeConverter is ugly
and dangerous - all know-how of how to handle type coercion needs to
stay in
org.apache.shindig.expressions (by a ValueExpression wrapper, if
necessary), not
into other code. Don't assume that all expression evaluation happens
in
DefaultTemplateProcessor.
The critical thing is that Opensocial spec compliance is paramount,
*not* TCK,
so commenting out existing tests is a blocker.
A major plus would be implementing this such that the choice of JUEL
vs. Jasper
is a Guice binding - support *both*, and let downstream vendors choose
when/if
to switch. That shouldn't be that hard - and if we reach a point
where Jasper
is clearly the right way to go, we can kill the JUEL support. In
particular, it
would let us do real-world testing of Jasper. For example, it's
possible that
Jasper uses less CPU but more memory than JUEL, and for some
deployments that
may be a critical difference.
http://codereview.appspot.com/1376043/diff/1/4
File
java/common/src/main/java/org/apache/shindig/expressions/ShindigTypeConverter.java
(right):
http://codereview.appspot.com/1376043/diff/1/4#newcode54
java/common/src/main/java/org/apache/shindig/expressions/ShindigTypeConverter.java:54:
public static <T> T convert(Object obj, Class<T> type) throws
ELException {
Making these functions static is bad - best practices in Guice land is
to avoid
statics. Note the @Inject above - you're breaking anyone that's
chosen to
override the type converter.
If you need to call this code from somewhere that didn't need it
before, the
proper fix is injecting an instance of this class into the target.
http://codereview.appspot.com/1376043/diff/1/5
File
java/common/src/test/java/org/apache/shindig/expressions/ExpressionsTest.java
(right):
http://codereview.appspot.com/1376043/diff/1/5#newcode134
java/common/src/test/java/org/apache/shindig/expressions/ExpressionsTest.java:134:
/*...@test
if you're going to suppress a test in JUnit 4, never comment it out.
Add the
@Ignore annotation, and include a String explaining why the tests is
ignored.
That said, these tests exist for a reason - they are required to
comply with the
Opensocial specification. Switching to a faster EL engine is a good
thing, but
you can only do so if you can make these tests pass. It's a
non-starter if they
don't (or you have to suppress them).
http://codereview.appspot.com/1376043/diff/1/5#newcode223
java/common/src/test/java/org/apache/shindig/expressions/ExpressionsTest.java:223:
ValueExpression expr= null;
please avoid tabs in all Shindig code.
http://codereview.appspot.com/1376043/diff/1/8
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/DefaultTemplateProcessor.java
(right):
http://codereview.appspot.com/1376043/diff/1/8#newcode124
java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/DefaultTemplateProcessor.java:124:
resolver.setCtx(this.elContext);
Comments as to what this is for? Why is the ELContext passed to the
resolver
ever wrong?
http://codereview.appspot.com/1376043/diff/1/8#newcode487
java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/DefaultTemplateProcessor.java:487:
Object result = expr.getValue(elContext);
shindig code pretty uniformly uses { } around all branches
http://codereview.appspot.com/1376043/diff/1/8#newcode493
java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/DefaultTemplateProcessor.java:493:
return defaultValue;
call this function only once
http://codereview.appspot.com/1376043/diff/1/8#newcode494
java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/DefaultTemplateProcessor.java:494:
}
odd indents
http://codereview.appspot.com/1376043/diff/1/9
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/TemplateELResolver.java
(right):
http://codereview.appspot.com/1376043/diff/1/9#newcode57
java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/TemplateELResolver.java:57:
public void setCtx(ELContext ctx) {
Name this method setContext(), or setELContext(); eschew
abbreviations.
That said: can you add Javadoc explaining the conditions under which
you'd need
this, or why it's necessary below? It's highly unusual to ignore the
ELContext
passed to a function (and why only in some ELResolver APIs?).
http://codereview.appspot.com/1376043/diff/1/9#newcode58
java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/TemplateELResolver.java:58:
this.ctx = ctx;
odd indent here.
http://codereview.appspot.com/1376043/diff/1/9#newcode98
java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/TemplateELResolver.java:98:
context = ctx;
Add a comment explaining what's going on here.
http://codereview.appspot.com/1376043/show
