Hi Fargo, I am still polishing up the patch, but could you take a look as to whether the changes are in the right direction or not?
http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java (right): http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java#newcode187 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:187: // Regardless what happens, inject a copy of the first node, On 2011/02/01 13:40:30, anupama.dutta wrote:
Update the comment to indicat that we inject as many copies of the
first node as
needed, with the new (concat) URIs.
Done. http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java (right): http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java#newcode40 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java:40: @Inject(optional = true) On 2011/02/01 13:40:30, anupama.dutta wrote:
Fix indentation on this line and the next.
Done. http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java#newcode127 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java:127: /* On 2011/02/01 13:40:30, anupama.dutta wrote:
Remove commented block.
Done. http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java (right): http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java#newcode153 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:153: if(batchUris != null && uriBuilder != ctx.makeQueryParams(null, null)) { On 2011/02/01 13:40:30, anupama.dutta wrote:
Space before (
Done. http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java#newcode166 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:166: for(UriBuilder cocnatUriBuilder : uriBuilders) { On 2011/02/01 13:40:30, anupama.dutta wrote:
cocnatUriBuilder -> concatUriBuilder
Done. http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java#newcode168 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:168: if(version != null) { On 2011/02/01 13:40:30, anupama.dutta wrote:
Indentation off and space needed before (
Done. http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java File java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java (right): http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java#newcode140 java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:140: assertNull(uri.getUris().get(0).getScheme()); On 2011/02/01 13:40:30, anupama.dutta wrote:
I think you should also assert that the size of getUris() is exactly 1
before
each of these blocks? Similar asserts for size of getUris() might be
needed in
other tests as well.
Done. http://codereview.appspot.com/3734041/
