You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by Henry Saputra <hs...@apache.org> on 2011/09/16 20:16:57 UTC
Review Request: SHINDIG-1622 Add support for optional "method" parameter in
the LinkSpec implementation
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1928/
-----------------------------------------------------------
Review request for shindig.
Summary
-------
The <Link> representation LinkSpec class missing "method" optional property as defined in the OpenSocial specs: http://opensocial-resources.googlecode.com/svn/spec/2.0/Core-Gadget.xml#LifeCycleEvents
This addresses bug SHINDIG-1622.
https://issues.apache.org/jira/browse/SHINDIG-1622
Diffs
-----
trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/LinkSpec.java 1171202
trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/spec/LinkSpecTest.java 1171202
Diff: https://reviews.apache.org/r/1928/diff
Testing
-------
Updated the unit test for LinkSpec and pass.
Thanks,
Henry
Re: Review Request: SHINDIG-1622 Add support for optional "method" parameter
in the LinkSpec implementation
Posted by Henry Saputra <hs...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1928/
-----------------------------------------------------------
(Updated 2011-09-16 22:32:42.088063)
Review request for shindig.
Changes
-------
Check for valid method names based review from Stanton.
Summary
-------
The <Link> representation LinkSpec class missing "method" optional property as defined in the OpenSocial specs: http://opensocial-resources.googlecode.com/svn/spec/2.0/Core-Gadget.xml#LifeCycleEvents
This addresses bug SHINDIG-1622.
https://issues.apache.org/jira/browse/SHINDIG-1622
Diffs (updated)
-----
trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java 1171202
trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/LinkSpec.java 1171202
trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/spec/LinkSpecTest.java 1171202
Diff: https://reviews.apache.org/r/1928/diff
Testing
-------
Updated the unit test for LinkSpec and pass.
Thanks,
Henry
Re: Review Request: SHINDIG-1622 Add support for optional "method" parameter
in the LinkSpec implementation
Posted by Henry Saputra <hs...@apache.org>.
> On 2011-09-16 21:58:27, Stanton Sievers wrote:
> > trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/LinkSpec.java, line 43
> > <https://reviews.apache.org/r/1928/diff/3/?file=41504#file41504line43>
> >
> > The spec says the value can be either "POST" or "GET". This implementation allows it to be anything. It could be left up to the implementor to sanitize the inputs or it could be done here. I don't feel strongly either way, just wanted to bring it up.
Good thinking, will add the check. Never trust caller =P
- Henry
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1928/#review1947
-----------------------------------------------------------
On 2011-09-16 19:53:07, Henry Saputra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1928/
> -----------------------------------------------------------
>
> (Updated 2011-09-16 19:53:07)
>
>
> Review request for shindig.
>
>
> Summary
> -------
>
> The <Link> representation LinkSpec class missing "method" optional property as defined in the OpenSocial specs: http://opensocial-resources.googlecode.com/svn/spec/2.0/Core-Gadget.xml#LifeCycleEvents
>
>
> This addresses bug SHINDIG-1622.
> https://issues.apache.org/jira/browse/SHINDIG-1622
>
>
> Diffs
> -----
>
> trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java 1171202
> trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/LinkSpec.java 1171202
> trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/spec/LinkSpecTest.java 1171202
>
> Diff: https://reviews.apache.org/r/1928/diff
>
>
> Testing
> -------
>
> Updated the unit test for LinkSpec and pass.
>
>
> Thanks,
>
> Henry
>
>
Re: Review Request: SHINDIG-1622 Add support for optional "method" parameter
in the LinkSpec implementation
Posted by Stanton Sievers <si...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1928/#review1947
-----------------------------------------------------------
Ship it!
One more thing to consider then this LGTM.
trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/LinkSpec.java
<https://reviews.apache.org/r/1928/#comment4428>
The spec says the value can be either "POST" or "GET". This implementation allows it to be anything. It could be left up to the implementor to sanitize the inputs or it could be done here. I don't feel strongly either way, just wanted to bring it up.
- Stanton
On 2011-09-16 19:53:07, Henry Saputra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1928/
> -----------------------------------------------------------
>
> (Updated 2011-09-16 19:53:07)
>
>
> Review request for shindig.
>
>
> Summary
> -------
>
> The <Link> representation LinkSpec class missing "method" optional property as defined in the OpenSocial specs: http://opensocial-resources.googlecode.com/svn/spec/2.0/Core-Gadget.xml#LifeCycleEvents
>
>
> This addresses bug SHINDIG-1622.
> https://issues.apache.org/jira/browse/SHINDIG-1622
>
>
> Diffs
> -----
>
> trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java 1171202
> trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/LinkSpec.java 1171202
> trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/spec/LinkSpecTest.java 1171202
>
> Diff: https://reviews.apache.org/r/1928/diff
>
>
> Testing
> -------
>
> Updated the unit test for LinkSpec and pass.
>
>
> Thanks,
>
> Henry
>
>
Re: Review Request: SHINDIG-1622 Add support for optional "method" parameter
in the LinkSpec implementation
Posted by Henry Saputra <hs...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1928/
-----------------------------------------------------------
(Updated 2011-09-16 19:53:07.819163)
Review request for shindig.
Changes
-------
Upload new diff based on review comment.
Summary
-------
The <Link> representation LinkSpec class missing "method" optional property as defined in the OpenSocial specs: http://opensocial-resources.googlecode.com/svn/spec/2.0/Core-Gadget.xml#LifeCycleEvents
This addresses bug SHINDIG-1622.
https://issues.apache.org/jira/browse/SHINDIG-1622
Diffs (updated)
-----
trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java 1171202
trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/LinkSpec.java 1171202
trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/spec/LinkSpecTest.java 1171202
Diff: https://reviews.apache.org/r/1928/diff
Testing
-------
Updated the unit test for LinkSpec and pass.
Thanks,
Henry
Re: Review Request: SHINDIG-1622 Add support for optional "method" parameter
in the LinkSpec implementation
Posted by Henry Saputra <hs...@apache.org>.
> On 2011-09-16 18:47:49, Stanton Sievers wrote:
> > Do you know who is taking the initiative to implement the usage of "method" in Shindig?
Not sure, its a community project so anyone could just do it.
> On 2011-09-16 18:47:49, Stanton Sievers wrote:
> > trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/LinkSpec.java, line 74
> > <https://reviews.apache.org/r/1928/diff/2/?file=41488#file41488line74>
> >
> > Is there a particular reason getMethod() isn't being added to GadgetsHandlerApi.LinkSpec?
No reason other than forget cause its too many duplicate classes for different purpose =(
I will add it in the code.
- Henry
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1928/#review1936
-----------------------------------------------------------
On 2011-09-16 18:16:57, Henry Saputra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1928/
> -----------------------------------------------------------
>
> (Updated 2011-09-16 18:16:57)
>
>
> Review request for shindig.
>
>
> Summary
> -------
>
> The <Link> representation LinkSpec class missing "method" optional property as defined in the OpenSocial specs: http://opensocial-resources.googlecode.com/svn/spec/2.0/Core-Gadget.xml#LifeCycleEvents
>
>
> This addresses bug SHINDIG-1622.
> https://issues.apache.org/jira/browse/SHINDIG-1622
>
>
> Diffs
> -----
>
> trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/LinkSpec.java 1171202
> trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/spec/LinkSpecTest.java 1171202
>
> Diff: https://reviews.apache.org/r/1928/diff
>
>
> Testing
> -------
>
> Updated the unit test for LinkSpec and pass.
>
>
> Thanks,
>
> Henry
>
>
Re: Review Request: SHINDIG-1622 Add support for optional "method" parameter
in the LinkSpec implementation
Posted by Henry Saputra <hs...@apache.org>.
> On 2011-09-16 18:47:49, Stanton Sievers wrote:
> > trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/LinkSpec.java, line 74
> > <https://reviews.apache.org/r/1928/diff/2/?file=41488#file41488line74>
> >
> > Is there a particular reason getMethod() isn't being added to GadgetsHandlerApi.LinkSpec?
>
> Henry Saputra wrote:
> No reason other than forget cause its too many duplicate classes for different purpose =(
>
> I will add it in the code.
Actually the GadgetHandler code no longer has reference to the GadgetsHandlerApi.LinkSpec. Do you know where its used now?
- Henry
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1928/#review1936
-----------------------------------------------------------
On 2011-09-16 18:16:57, Henry Saputra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1928/
> -----------------------------------------------------------
>
> (Updated 2011-09-16 18:16:57)
>
>
> Review request for shindig.
>
>
> Summary
> -------
>
> The <Link> representation LinkSpec class missing "method" optional property as defined in the OpenSocial specs: http://opensocial-resources.googlecode.com/svn/spec/2.0/Core-Gadget.xml#LifeCycleEvents
>
>
> This addresses bug SHINDIG-1622.
> https://issues.apache.org/jira/browse/SHINDIG-1622
>
>
> Diffs
> -----
>
> trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/LinkSpec.java 1171202
> trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/spec/LinkSpecTest.java 1171202
>
> Diff: https://reviews.apache.org/r/1928/diff
>
>
> Testing
> -------
>
> Updated the unit test for LinkSpec and pass.
>
>
> Thanks,
>
> Henry
>
>
Re: Review Request: SHINDIG-1622 Add support for optional "method" parameter
in the LinkSpec implementation
Posted by Henry Saputra <hs...@apache.org>.
> On 2011-09-16 18:47:49, Stanton Sievers wrote:
> > trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/LinkSpec.java, line 74
> > <https://reviews.apache.org/r/1928/diff/2/?file=41488#file41488line74>
> >
> > Is there a particular reason getMethod() isn't being added to GadgetsHandlerApi.LinkSpec?
>
> Henry Saputra wrote:
> No reason other than forget cause its too many duplicate classes for different purpose =(
>
> I will add it in the code.
>
> Henry Saputra wrote:
> Actually the GadgetHandler code no longer has reference to the GadgetsHandlerApi.LinkSpec. Do you know where its used now?
Never mind its refactored to GadgetsHandlerService =)
- Henry
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1928/#review1936
-----------------------------------------------------------
On 2011-09-16 18:16:57, Henry Saputra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1928/
> -----------------------------------------------------------
>
> (Updated 2011-09-16 18:16:57)
>
>
> Review request for shindig.
>
>
> Summary
> -------
>
> The <Link> representation LinkSpec class missing "method" optional property as defined in the OpenSocial specs: http://opensocial-resources.googlecode.com/svn/spec/2.0/Core-Gadget.xml#LifeCycleEvents
>
>
> This addresses bug SHINDIG-1622.
> https://issues.apache.org/jira/browse/SHINDIG-1622
>
>
> Diffs
> -----
>
> trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/LinkSpec.java 1171202
> trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/spec/LinkSpecTest.java 1171202
>
> Diff: https://reviews.apache.org/r/1928/diff
>
>
> Testing
> -------
>
> Updated the unit test for LinkSpec and pass.
>
>
> Thanks,
>
> Henry
>
>
Re: Review Request: SHINDIG-1622 Add support for optional "method" parameter
in the LinkSpec implementation
Posted by Stanton Sievers <si...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1928/#review1936
-----------------------------------------------------------
Do you know who is taking the initiative to implement the usage of "method" in Shindig?
trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/LinkSpec.java
<https://reviews.apache.org/r/1928/#comment4421>
Is there a particular reason getMethod() isn't being added to GadgetsHandlerApi.LinkSpec?
- Stanton
On 2011-09-16 18:16:57, Henry Saputra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1928/
> -----------------------------------------------------------
>
> (Updated 2011-09-16 18:16:57)
>
>
> Review request for shindig.
>
>
> Summary
> -------
>
> The <Link> representation LinkSpec class missing "method" optional property as defined in the OpenSocial specs: http://opensocial-resources.googlecode.com/svn/spec/2.0/Core-Gadget.xml#LifeCycleEvents
>
>
> This addresses bug SHINDIG-1622.
> https://issues.apache.org/jira/browse/SHINDIG-1622
>
>
> Diffs
> -----
>
> trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/LinkSpec.java 1171202
> trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/spec/LinkSpecTest.java 1171202
>
> Diff: https://reviews.apache.org/r/1928/diff
>
>
> Testing
> -------
>
> Updated the unit test for LinkSpec and pass.
>
>
> Thanks,
>
> Henry
>
>