Proposal for 1.0: os:Variable tag - update 1

Updated patch addressing comments and discussion thread

[levik, 2009-12-10 17:59:13]

Thanks for putting this together, Chris. I think largely my comments are
cosmetic except for two points:

 - I don't see why variables created in inline templates should be global when
custom tag-created vars are temporary. I think the right thing to do is to have
them all be temporary.

 - I *really* don't like the ability to put content into <os:Var>, especially
for the purposes of sticking markup into a variable.

http://codereview.appspot.com/169044/diff/1/2
File OpenSocial-Templating.xml (right):

http://codereview.appspot.com/169044/diff/1/2#newcode320
OpenSocial-Templating.xml:320: with the os:Var tag.  This tag may also be used
data pipelining to declare
"...used *in* data pipelining *blocks/sections*..." ?

http://codereview.appspot.com/169044/diff/1/2#newcode325
OpenSocial-Templating.xml:325: It may be used in inline custom tag templates
(temporary) or inline templates or
Move "(temporary)" after "inline templates", to indicate those vars are temp as
well.

http://codereview.appspot.com/169044/diff/1/2#newcode326
OpenSocial-Templating.xml:326: data-pipeline sections (global).  The element
supports three attributes:
I feel like we should have an example of <os:Var> before discussing its
attributes - this will make the spec easier to visualize.

http://codereview.appspot.com/169044/diff/1/2#newcode330
OpenSocial-Templating.xml:330: <t> @key: Required. defines the string name of
the variable.  Expressions may not be used.</t>
I am not sure I like how "Expressions may not be used" sounds (though I agree
with the intent). Unfortunately I can't come up with a better way to phrase it.

http://codereview.appspot.com/169044/diff/1/2#newcode331
OpenSocial-Templating.xml:331: <t> @value: Optional. Defines the value of the
variable.   Expressions may be used.</t>
Same here.

http://codereview.appspot.com/169044/diff/1/2#newcode335
OpenSocial-Templating.xml:335: When the @value attribute is not specified, the
contents of the &lt;os:Var&gt; tag is used as the value.
Is this neccessary? I thought we did away with this point. Allowing <os:Var> to
have content opens us up to various ambiguities like allowing/disallowing
repeaters and conditionals inside.

http://codereview.appspot.com/169044/diff/1/2#newcode343
OpenSocial-Templating.xml:343: <t>Attempt to parse it as a JSON value</t>
"Attempt to parse the value as a JSON string and return the resulting object if
successful"

http://codereview.appspot.com/169044/diff/1/2#newcode344
OpenSocial-Templating.xml:344: <t>If all other processing does not fully resolve
the value, return as a string</t>
What does "fully" mean here?

http://codereview.appspot.com/169044/diff/1/2#newcode364
OpenSocial-Templating.xml:364: declared in a data pipelineing section may not
re-declare an existing
I think <os:Var> as it pertains to Data Pipelining should be specified in the DP
document - it doesn't belong here. If anything, we can reference that definition
here to say the support is templates is similar to one in DP. 

Otherwise we're creating a circular dependency of specs.

http://codereview.appspot.com/169044/diff/1/2#newcode372
OpenSocial-Templating.xml:372: </t>
I'm against this - template rendering should not have side effects in the global
scope. I think variables in inline templates should behave same as ones in
custom tag templates. What's the reason to have an inconsistency?

http://codereview.appspot.com/169044/diff/1/2#newcode435
OpenSocial-Templating.xml:435: overwritten by the latest value.  After running
the below code, the variable
"below code" -> "code below"?

http://codereview.appspot.com/169044/diff/1/2#newcode486
OpenSocial-Templating.xml:486: 
I'm against such use of <os:Var> (as I am against allowing value to be derived
from tag content). I think <os:Var> should be focused on working with JSON data,
not HTML markup. We already have mechanisms for working with markup: <os:Html>
and <os:Render> - let's not create another way to do what can already be done by
existing spec features.

[goosemanjack, 2010-01-12 22:02:30]

Modifications discussed in thread to leave contentious items undefined

[levik, 2010-01-13 16:55:21]

Chris, I think a number of comments I made were unadressed:
http://codereview.appspot.com/169044/diff2/1:4001/5001

A lot of these were either cosmetic, or agreed upon in the threads.

Also, I'm not sure why the processing order was switched around so that JSON
parsing is attempted *before *EL expression processing... This seems to make
it impossible to populate custom JSON objects with runtime data...

Finally, I believe we agreed that <os:Var>s would be attached to the ${Cur}
variable - making them "temporary" in all scopes. This change is not
reflected yet.

On Tue, Jan 12, 2010 at 5:02 PM, <goosemanjack@gmail.com> wrote:

> Modifications discussed in thread to leave contentious items undefined
>
> http://codereview.appspot.com/169044
>

[goosemanjack, 2010-01-21 00:28:45]

Moving bulk of os:Var into Core-Gadget and referencing from Template spec

[goosemanjack, 2010-01-21 00:31:17]

Per Evan's comments, leaving mute on allowing EL in JSON objects