feat(AstParser): Made the AST parser private to the scope (was: fix(parser/eval): fix for '"s" + ("m"|filter) + "e"') #758

vicb · 2014-03-18T18:44:16Z

closes #755

That's a 3-liner only but it really doesn't reflect the time spent in debugging !

vicb · 2014-03-18T18:46:02Z

@mhevery could you please check and merge so that I can rebase #753 which works well on top of that.

vicb · 2014-03-18T19:15:09Z

Seems like the Travis takes a long time, but it's ok on my repo https://travis-ci.org/vicb/angular.dart/builds/21039875

chalin · 2014-03-18T21:03:34Z

I believe that @pavelgj took out support for filter expressions not too long ago:

5bcea64, fix(parser): disallow filters in a chain and inside expressions

This was as a follow up to an issue that Günter Zöchbauer had raised, if I recall correctly (though I cannot find the original issue).

vicb · 2014-03-18T21:10:56Z

thanks for the ref @chalin.

@pavelgj I would appreciate your comments on this PR (it is needed for #753.)

pavelgj · 2014-03-18T21:17:27Z

Yes, this feature was removed as it was never meant to exist in the first place. This syntax IMHO is very unintuitive.

chalin · 2014-03-18T21:23:23Z

@pavelgj, the pipe syntax is used in other languages (see this post for some examples).
@vicb, given that filters are pure, are you sure that change detection works correctly with filters appearing as inner expressions (i.e. other than a top-level expression)? In particular with a list or map being passed as an argument to the filter (and the list or map being changed while its identity is preserved)?

vicb · 2014-03-18T21:35:50Z

@pavelgj I don't get what's unintuitive, could explain give some more details. This is what we have been discussing with @mhevery to make the AstParser private to the scope.

@chalin would you mind PR a test here ?

pavelgj · 2014-03-18T21:41:52Z

It's just my opinion, but @mhevery asked me to remove it to simplify things for change detection and deferred loading. If things changed I'm OK with bringing it back.

pavelgj · 2014-03-19T00:46:04Z

Why I find it unintuitive: when I read foo | bar the first thing that comes to mind is foo BITWISE_OR bar. Shell script is not my primary programming language. :)

vicb · 2014-03-19T06:28:56Z

Thanks for the explanation. Didn't get that as a lot of templating languages I know use |. One argument could be it's hard to find on an android keyboard :)

vicb · 2014-03-19T14:03:53Z

At least I noticed that CallReflective._eval and CallReflective._evaluateUncached do not accept filters as a parameter.

@kasperl I've handled to most trivial cases in the last commit. Would you mind handling other cases ? If yes, you can send a PR to my branch, I'll merge it.

I have not handled CallReflective nor assign(). I think the latest would need the filters as it eval.

Also it should not be possible to make the FilterMap a required param because of inheritence ?

mhevery · 2014-03-19T18:06:17Z

Please revert 5bcea64 instead of this PR.

vicb · 2014-03-19T18:26:06Z

@mhevery 5bcea64 is two folds:

disabling filters in expressions,
disabling filters in chains.

The first point is already revert in this PR, is there a need to revert 2 ?

vicb · 2014-03-19T18:31:24Z

I'll merge "feat(AstParser): Made the AST parser private to the scope #753" here

vicb · 2014-03-19T19:53:17Z

@mhevery I question I forgot to ask is why filter are |filter:arg1:arg2 ?
I once again prefer Jinja style, ie |filter(arg1, arg2).

chalin · 2014-03-19T19:57:05Z

+1 for Jinja style (which also compatible with what is proposed for Dart).

mhevery · 2014-03-19T23:40:26Z

We already have lots of People using the current style so unless there are technical reasons, I don't think we should be changing the syntax. The current syntax is from Django, historical reasons.

chalin · 2014-03-19T23:43:58Z

Support the Jinja style as an alternative syntax?

See: 5bcea64 Closes dart-archive#758

mhevery · 2014-03-20T05:16:53Z

I have tried to merge this in, but ran into issues. Could you look at my work and correct the remaining issues: https://github.com/mhevery/angular.dart/tree/pr-758

zoechi · 2014-03-20T07:00:14Z

I find this |filter:arg1:arg2 very unintuitive.
Each time I didn't use filters for a while I have to look up the syntax.
Pre 1.0 would be a good time to drop 'for historical reasons'
Just my 0.1c

closes dart-archive#755

closes dart-archive#648

This commit also: - remove the previously registered watch, - add an expression cache

To make sure a string is always returned

closes dart-archive#739 This changes is a BC break: - the "readOnly" parameter has been renamed to "canChangeModel", - the semantic is inversed Before: // Default to digest (readOnly = false by default) scope.watch(exp, fn); // Explicit flush scope.watch(exp, fn, readOnly: true); After // Default to digest (canChangeModel = true by default) scope.watch(exp, fn); // Explicit flush scope.watch(exp, fn, canChangeModel: false);

vicb · 2014-03-20T12:21:42Z

@mhevery I think this PR is now OK:

I cherry picked your commit which make filters a required arg,
I have tried to remove my "optimization" that removes empty strings from expression (please check the penultimate commit),
I have added the last commit to rename scope.watch(readOnly) to scope.watch(canChangeModel)

Should be ready to be merged !

chalin · 2014-03-20T15:59:17Z

@zoechi, #772 could use your 👍 if you agree with it.

zoechi · 2014-03-20T16:44:05Z

of course, as many as you want 👍 👍 👍 👍 👍 (never saw how easy they are to insert).

I'm never sure if I annoy anybody if I add to many comments here.
It seemed to go in the right direction without repeating my opinion anyway 😉
Great work, anybody!

@chalin Is there any reason you referenced #722 ?

chalin · 2014-03-20T16:47:39Z

(Oups, mistyped: meant #772 ... )

vicb · 2014-03-20T18:21:03Z

hum... I have this strange bug

Chrome 33.0.1750 (Linux) ERROR
  Some of your tests did a full page reload!

Thanks I currently fail to narrow down :(

vicb · 2014-03-22T14:45:27Z

Closing, I'll open an other one, cleaned and rebased

vicb added cla: yes and removed cla: no labels Mar 18, 2014

vicb mentioned this pull request Mar 19, 2014

[WIP]feat(AstParser): Made the AST parser private to the scope #753

Closed

vicb changed the title ~~fix(parser/eval): fix for '"s" + ("m"|filter) + "e"'~~ feat(AstParser): Made the AST parser private to the scope (was: fix(parser/eval): fix for '"s" + ("m"|filter) + "e"') Mar 19, 2014

vicb mentioned this pull request Mar 19, 2014

Make AstParser an implementation detail of Scope #648

Closed

mhevery added a commit to mhevery/angular.dart that referenced this pull request Mar 20, 2014

feat(filters): revert filter being restricted to top level

8e67468

See: 5bcea64 Closes dart-archive#758

vicb mentioned this pull request Mar 20, 2014

[Q] Should with support the filter syntax |filter(arg1, arg2) #772

Closed

vicb added 3 commits March 20, 2014 11:56

fix(parser/eval): fix for '"s" + ("m"|filter) + "e"'

cbf94f0

closes dart-archive#755

feat(AstParser): Made the AST parser private to the scope

5ac4190

closes dart-archive#648

feat(scope): watch watches expressions (String) only

7bc625b

vicb and others added 7 commits March 20, 2014 12:38

feat(directives): remove refs to AstParser, watch strings

31aa004

feat(interpolate): the service now returns an expression only

7f921d3

refactor(ngPluralize): minor tweaks and code style

508d35a

feat(ngPluralize): support the new interpolation service

4e53f73

This commit also: - remove the previously registered watch, - add an expression cache

fix(filters): make filters required and pass it to all evals

2617bb7

fix(interpolate): do not try to optimize by removing empty strings

8fa8de9

To make sure a string is always returned

vicb mentioned this pull request Mar 20, 2014

Optimize ngRepeat by using the new CD #751

Closed

2 tasks

feat(stringify): Add the stringify filter to convert obj to strings

18a48c3

vicb added 2 commits March 20, 2014 19:48

sq mustache

ea955e6

almost there....

47153ed

vicb closed this Mar 22, 2014

vicb mentioned this pull request Mar 22, 2014

feat(AstParser): Made the AST parser private to the scope #788

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(AstParser): Made the AST parser private to the scope (was: fix(parser/eval): fix for '"s" + ("m"|filter) + "e"') #758

feat(AstParser): Made the AST parser private to the scope (was: fix(parser/eval): fix for '"s" + ("m"|filter) + "e"') #758

vicb commented Mar 18, 2014

vicb commented Mar 18, 2014

vicb commented Mar 18, 2014

chalin commented Mar 18, 2014

vicb commented Mar 18, 2014

pavelgj commented Mar 18, 2014

chalin commented Mar 18, 2014

vicb commented Mar 18, 2014

pavelgj commented Mar 18, 2014

pavelgj commented Mar 19, 2014

vicb commented Mar 19, 2014

vicb commented Mar 19, 2014

mhevery commented Mar 19, 2014

vicb commented Mar 19, 2014

vicb commented Mar 19, 2014

vicb commented Mar 19, 2014

chalin commented Mar 19, 2014

mhevery commented Mar 19, 2014

chalin commented Mar 19, 2014

mhevery commented Mar 20, 2014

zoechi commented Mar 20, 2014

vicb commented Mar 20, 2014

chalin commented Mar 20, 2014

zoechi commented Mar 20, 2014

chalin commented Mar 20, 2014

vicb commented Mar 20, 2014

vicb commented Mar 22, 2014

feat(AstParser): Made the AST parser private to the scope (was: fix(parser/eval): fix for '"s" + ("m"|filter) + "e"') #758

feat(AstParser): Made the AST parser private to the scope (was: fix(parser/eval): fix for '"s" + ("m"|filter) + "e"') #758

Conversation

vicb commented Mar 18, 2014

vicb commented Mar 18, 2014

vicb commented Mar 18, 2014

chalin commented Mar 18, 2014

vicb commented Mar 18, 2014

pavelgj commented Mar 18, 2014

chalin commented Mar 18, 2014

vicb commented Mar 18, 2014

pavelgj commented Mar 18, 2014

pavelgj commented Mar 19, 2014

vicb commented Mar 19, 2014

vicb commented Mar 19, 2014

mhevery commented Mar 19, 2014

vicb commented Mar 19, 2014

vicb commented Mar 19, 2014

vicb commented Mar 19, 2014

chalin commented Mar 19, 2014

mhevery commented Mar 19, 2014

chalin commented Mar 19, 2014

mhevery commented Mar 20, 2014

zoechi commented Mar 20, 2014

vicb commented Mar 20, 2014

chalin commented Mar 20, 2014

zoechi commented Mar 20, 2014

chalin commented Mar 20, 2014

vicb commented Mar 20, 2014

vicb commented Mar 22, 2014