fix(parser): Allow access to Map properties in expressions #605

chalin · 2014-02-24T17:06:47Z

E.g. map.keys, map.length, etc. This is a proposed solution to #394.

mhevery · 2014-02-25T05:24:15Z

test/core/parser/parser_spec.dart

@@ -647,6 +647,16 @@ main() {
      });


+      it('should evaluate map item and field access', () {


Unfortunately this is only half the work. Same kinds of tests also have to work on scope watching, and I think right now, they would fail. Could you create a scope.watch('may.isEmpty') test to make sure it works there as well. Change detection takes a different path.

Ok. Will try to find previous example of a scope.watch() test as an example (probably in scope_spec.dart).

Done for map properties whose values are of a primitive type like map.length, map.isEmpty. Still need to work on map.keys and map.values but that will have to wait until midday tomorrow.

vicb · 2014-02-25T20:51:58Z

@chalin you should prepend "[WIP]" to the title of PRs which are work in progress. (And you should merge master into this judging by the commit diff list)

chalin · 2014-02-25T22:23:57Z

@vicb : thanks for letting me know about the process; I edited the title to start with "[WIP]". (As for your other comment, you are suggesting that I merge rather than rebase master into my branch?)

vicb · 2014-02-25T23:35:35Z

rebasing is not preferred for "long running" branches, which I why I would advice merging master into your branch. You can still rebase later when the code stabilizes and the PR is ready to be merged into master.

chalin · 2014-02-26T00:07:52Z

Thanks for the link to the "perils of rebasing". Would it be best to cancel this PR and resubmit a new one?

vicb · 2014-02-26T00:15:22Z

Would it be best to cancel this PR and resubmit a new one?

why ?

chalin · 2014-02-26T00:51:30Z

Because this PR now muddles the commits actually belonging to this PR with those that were brought in via the rebase.

vicb · 2014-02-26T08:08:21Z

don't know exactly what happened with this PR. You should check the git graph log.
A force push should be enough if really required (it will mess history), no need to re-open a new PR.

chalin · 2014-02-26T13:05:37Z

Wouldn't it be preferable to open a new PR than to mess with history?

vicb · 2014-02-26T13:08:05Z

Up to you but this one already has some linked issues & comments so I would keep it (and some commits might disappear from the diff once master is merged here)

mhevery · 2014-03-06T06:27:32Z

Sorry, that this PR got stale. Could you rebase it to the latest codebase?

chalin · 2014-03-06T14:29:33Z

Yes, that is the plan. I would like to understand the new change detection alg first though ... any chance that this post might get answered soon?

vicb · 2014-03-06T14:47:10Z

Patrice, have you checked this doc

chalin · 2014-03-06T17:38:08Z

Thanks for the pointer Vic!

chalin · 2014-03-08T19:19:37Z

Having looked at the code it seems that any watched expression that produces a new (non identical()) object that is not a String, will fail to stabilize under the new change detection algorithm (CDA).

@vicb, @mhevery, can you confirm?

If so, then while map properties like length can be supported, but map.keys and map.values will not be useful under the next CDA.

mhevery · 2014-03-13T22:10:16Z

That is correct, and that is by intention. Strings have an exception due to the way Strings work in JS.

mhevery · 2014-03-13T22:12:44Z

Should this be kept open, or closed?

chalin · 2014-03-13T22:17:12Z

Open. I think it would still be useful to support, e.g., map.length. And one of your comments on another thread gave me an idea about dealing with map.keys. If you agree, I'll clean this PR up.

chalin · 2014-03-14T12:57:36Z

Done. (Actually, this is pretty much the solution as committed on Feb 27. In the interim I had been trying to get map.keys to work which I now know isn't possible under the current CDA.)

vicb · 2014-03-14T13:14:37Z

lib/change_detection/dirty_checking_change_detector.dart

@@ -424,7 +425,9 @@ class DirtyCheckingRecord<H> implements Record<H>, WatchRecord<H> {
      return;
    }

-    if (obj is Map) {
+    if (obj is Map && (!isMapProperty(field)


|| should be moved up - maybe everything fits 80 chars ?

vicb · 2014-03-14T13:16:12Z

Could you please follow guidelines for commit comments (and probably squash them ?)

chalin · 2014-03-14T13:58:51Z

I took a few tries (GitHub app wasn't helping), but I think I managed to squash the commits, fix if condition to fit on one line and adjusted the commit message.

vicb · 2014-03-14T17:00:35Z

@chalin I don't know about GH app (I'm running on Ubuntu) - git-cola is very nice but of course no GH integration.

chalin · 2014-03-15T11:54:22Z

I'm just falling back the cmd line most of the time now. Might give git-cola a try, thanks. Btw, anything else for me to do on this PR that is preventing it from being merged?

vicb · 2014-03-17T10:24:46Z

bin/parser_generator_for_spec.dart

@@ -101,6 +101,11 @@ main(arguments) {
      'items[1].name',
      'list[3] = 2',
      'map["square"] = 6',
+      'map.isEmpty',
+      'map.isNotEmpty',
+      'map.keys',


should keys and values still be here ?

Yes: I have a small proposal to share with the team related to .keys and .values. Do you still want me to take them out for now?

vicb · 2014-03-17T10:25:34Z

lib/tools/parser_generator/dart_code_gen.dart


  String lookup(String key) {
    String name = _computeName(key);
    if (helpers.containsKey(key)) return name;
    helpers[key] = isReservedWord(key)
        ? templateForReserved(name, key)
+        : isMapProperty(key) && templateWithoutMapTest != null
+          ? templateWithoutMapTest(name, key)


indentation

vicb · 2014-03-17T10:26:53Z

@chalin are there usecases for .values and .keys ? souldn't we only implement a | keys filter instead ?

vicb · 2014-03-17T10:37:56Z

@chalin if the commit comment is right, we could access a random key with map.key or map['key'] ?

Then there could be a confusion for keys which have the same name as a map property. What about accessing properties with . only and keys with [] only, as in Dart ?

chalin · 2014-03-17T11:49:48Z

I agree with you entirely: which is why I had opened #606, Treat Dart maps like maps, not (JavaScript) objects with field access.

vicb · 2014-03-17T12:54:53Z

@chalin I would prefer to see:

support for keys and values removed (as it could cause confusion for now)
using only [] to access map keys (and . for map property).

May be you can submit an other PR that could be merged soon and then we'll discuss the remaining points ?

chalin · 2014-03-17T13:31:49Z

@vicb:

support for keys and values removed (as it could cause confusion for now)

Done.

using only [] to access map keys (and . for map property).

Agreed, but I worry that such a change would be controversial and hence delay this PR from being merged even more. Would you mind if I did this work under #606?

May be you can submit an other PR that could be merged soon and then we'll discuss the remaining points ?

Yes.

vicb · 2014-03-17T14:44:01Z

Agreed, but I worry that such a change would be controversial and hence delay this PR from being merged even more. Would you mind if I did this work under #606?

probably a good idea.

There is still an indentation issue, otherwise LGTM

chalin · 2014-03-17T15:07:58Z

Oups, forgot about it.
Done.

…ions Allow access to all `Map` properties like `map.length`, except for `keys` and `values`. Introduces changes to parser and change detection. Breaking change: map keys matching `Map` property names will now need to be accessed as, e.g., `map[“length”]`.

mhevery · 2014-03-19T04:02:51Z

I apologize for taking so long to respond to this. See my current thinking here: #397 (comment)

I am going to close this PR because it is not aligned with the direction as explained in the comment above.

chalin · 2014-03-19T11:39:25Z

Ok np, but if you take a quick look at the diff (+53/-8 lines) most of this PR is in support of your Step 1 since it adds tests to confirm that map field access works. only 3 tests (!isMapProperty(name)) would need to be removed to be in full alignment with your step 1.

chalin mentioned this pull request Feb 24, 2014

Create a filter which takes map and returns a list of key value pairs. Useful for iterating over Maps #394

Closed

chalin added cla: yes and removed cla: no labels Feb 24, 2014

This was referenced Feb 24, 2014

Allow Map.values in ng-repeat #397

Closed

Treat Dart maps like maps, not (JavaScript) objects with field access #606

Closed

mhevery reviewed Feb 25, 2014
View reviewed changes

vicb reviewed Mar 14, 2014
View reviewed changes

vicb reviewed Mar 17, 2014
View reviewed changes

mhevery closed this Mar 19, 2014

chalin deleted the patch-0224-map-property-access branch March 20, 2014 03:32

		@@ -647,6 +647,16 @@ main() {
		});


		it('should evaluate map item and field access', () {

fix(parser): Allow access to Map properties in expressions #605

fix(parser): Allow access to Map properties in expressions #605

Conversation

chalin commented Feb 24, 2014

mhevery Feb 25, 2014

Choose a reason for hiding this comment

chalin Feb 25, 2014

Choose a reason for hiding this comment

chalin Feb 25, 2014

Choose a reason for hiding this comment

vicb commented Feb 25, 2014

chalin commented Feb 25, 2014

vicb commented Feb 25, 2014

chalin commented Feb 26, 2014

vicb commented Feb 26, 2014

chalin commented Feb 26, 2014

vicb commented Feb 26, 2014

chalin commented Feb 26, 2014

vicb commented Feb 26, 2014

mhevery commented Mar 6, 2014

chalin commented Mar 6, 2014

vicb commented Mar 6, 2014

chalin commented Mar 6, 2014

chalin commented Mar 8, 2014

mhevery commented Mar 13, 2014

mhevery commented Mar 13, 2014

chalin commented Mar 13, 2014

chalin commented Mar 14, 2014

vicb Mar 14, 2014

Choose a reason for hiding this comment

vicb commented Mar 14, 2014

chalin commented Mar 14, 2014

vicb commented Mar 14, 2014

chalin commented Mar 15, 2014

vicb Mar 17, 2014

Choose a reason for hiding this comment

chalin Mar 17, 2014

Choose a reason for hiding this comment

vicb Mar 17, 2014

Choose a reason for hiding this comment

vicb commented Mar 17, 2014

vicb commented Mar 17, 2014

chalin commented Mar 17, 2014

vicb commented Mar 17, 2014

chalin commented Mar 17, 2014

vicb commented Mar 17, 2014

chalin commented Mar 17, 2014

mhevery commented Mar 19, 2014

chalin commented Mar 19, 2014