Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(11)

Issue 124104: Update to JS and PHP for args, predicates (with .if), context in formatter, predicates...

Can't Edit
Can't Publish+Mail
Start Review
Created:
16 years, 6 months ago by sroussey
Modified:
10 years, 4 months ago
Reviewers:
andychu
CC:
json-template_googlegroups.com
Visibility:
Public.

Patch Set 1 #

Total comments: 15
Unified diffs Side-by-side diffs Delta from patch set Stats (+468 lines, -204 lines) Patch
M javascript/json-template.js View 8 chunks +180 lines, -98 lines 14 comments Download
M jsontemplate_test.py View 3 chunks +9 lines, -9 lines 1 comment Download
M php/jsontemplate.php View 18 chunks +271 lines, -97 lines 0 comments Download
M php/jsontemplate_cmd.php View 1 chunk +8 lines, -0 lines 0 comments Download

Messages

Total messages: 2
sroussey
JS and PHP pass all 47 tests. In JS: - formatters always with val,context,args - ...
16 years, 6 months ago (2009-10-03 02:12:20 UTC) #1
andychu
16 years, 6 months ago (2009-10-03 05:16:02 UTC) #2
OK, great that you got this all working.

I'm going to be more picky about the JS code -- you can have more leeway on the
PHP code, as long as it passes the tests.  Although we should strive to make the
APIs as consistent as possible, allowing for language differences.

I still have to figure out exactly what I want in predicates.  I don't think
these tests can pass, because you have .if in the code, but no .if in the tests.

I will work on the details of predicates... if there is soemthing you want to
see other than what we've discussed let me know.

http://codereview.appspot.com/124104/diff/1/2
File javascript/json-template.js (right):

http://codereview.appspot.com/124104/diff/1/2#newcode48
Line 48: (META_ESCAPE[meta_right]||meta_right) +
Please hold off on this until it does what we discussed -- also a unit test
would be nice.  It's possible to have JS-specific unit tests in browser_test.py
although it's set up a little awkwardly.

http://codereview.appspot.com/124104/diff/1/2#newcode83
Line 83: return context.Lookup('base-url')+'/'+val;
I would like if this behaves like Python and is smart WRT leading/trailing
slashes, but this could bea  TODO

http://codereview.appspot.com/124104/diff/1/2#newcode97
Line 97: function JsonTemplateException(message) {
I'd rather not have this here yet, let's talk about it separately from the other
issues.

http://codereview.appspot.com/124104/diff/1/2#newcode126
Line 126: stacker:stack,
On 2009/10/03 02:12:20, sroussey wrote:
> Oops.. not needed, was there for firebug at one point...

So you should upload another snapshot with this removed rather than replying to
yourself :)

http://codereview.appspot.com/124104/diff/1/2#newcode183
Line 183: return frame.index;
On 2009/10/03 02:12:20, sroussey wrote:
> $index starting from 1, not zero. Can imagine if the W3C suggested that for
> <ol><li> items...

OK.  But you must have broken Python too then.

http://codereview.appspot.com/124104/diff/1/2#newcode228
Line 228: if (matches && (predicate = DEFAULT_PREDICATES[matches[1]])) {
I will have to think about this stuff... I suggested a more general API for
registering formatters/predicates in one of the bugs.  This doesn't match what's
in Python now.

http://codereview.appspot.com/124104/diff/1/2#newcode246
Line 246: if (predicate_target==null || predicate_target==undefined)
Please keep the style consistent and use spaces around ==, here and throughout.

http://codereview.appspot.com/124104/diff/1/2#newcode253
Line 253: predicate_target: predicate_target, // public attribute
I think that sections blocks have names and no predicates, while "if" blocks
have predicates but no names.

If that's the case then we should create a new "class" -- in Python I created
_PredicateSection and _Section, which derive from _AbstractSection.

http://codereview.appspot.com/124104/diff/1/2#newcode372
Line 372: var _SECTION_RE =
/^(?:(repeated)\s+)?section\s+(@|[A-Za-z0-9$_-]+)\s*$/;
I don't see why these changed.

http://codereview.appspot.com/124104/diff/1/2#newcode374
Line 374: var _CALL_RE = /^([A-Za-z0-9$_-]+)(?:\b\s*(.*?))?\s*$/;
As discussed I don't want to parse any arguments directly in the template
language.  This can be left for user code.  Perhaps I can come up with some more
examples in Python.

http://codereview.appspot.com/124104/diff/1/3
File jsontemplate_test.py (right):

http://codereview.appspot.com/124104/diff/1/3#newcode856
Line 856: class PredicatesTest(testy.Test):
Change description says you have .if, but I don't see any .if here.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b