Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PropertyPath in observe v0.11.0 still doesn't support Map indexers #20294

Closed
DartBot opened this issue Jul 31, 2014 · 7 comments
Closed

PropertyPath in observe v0.11.0 still doesn't support Map indexers #20294

DartBot opened this issue Jul 31, 2014 · 7 comments
Labels
area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. closed-obsolete Closed as the reported issue is no longer relevant

Comments

@DartBot
Copy link

DartBot commented Jul 31, 2014

This issue was originally filed by @jolleekin


  1. PropertyPath doesn't support Map indexers of type string

    import 'package:observe/observe.dart';

    main() {
      var p = new PropertyPath('resources["commands"]["+add"]');

      // Output: resources.commands["+add"]
      // Expect: resources["commands"]["+add"]
      print(p);
    }

"commands" is actually converted into a symbol while "+add" is left as is.
Ideally, indexers should be left as is whether they are Map indexers or List indexers.

  1. PropertyPath doesn't support Map indexers of type int

The following code only checks for integer List indexers and forgets about integer Map indexers.

    // path_observe.dart
    _getObjectProperty(object, property) {
      if (object == null) return null;

      if (property is int) {
        if (object is List && property >= 0 && property < object.length) {
          return object[property];
        }
      } else if (property is String) {
        return object[property];
      } else if (property is Symbol) {
        ...
      }
      ...
    }

What version of the product are you using?
observe 0.11.0

On what operating system?

What browser (if applicable)?

Please provide any additional information below.

@DartBot
Copy link
Author

DartBot commented Jul 31, 2014

This comment was originally written by @zoechi


is this related to https://code.google.com/p/dart/issues/detail?id=20286 ?

@DartBot
Copy link
Author

DartBot commented Aug 1, 2014

This comment was originally written by @jolleekin


I don't think so. Polymer doesn't use PropertyPath.

@sigmundch
Copy link
Member

Thanks for the detailed bug description. This makes sense now.

Re (1): that is also how the JS implementation is written. We have to follow up with them to make sure we are consistent.


Removed Priority-Unassigned label.
Added Priority-Medium, Pkg-Observe, Area-Pkg, PolymerMilestone-Next, Triaged labels.

@DartBot
Copy link
Author

DartBot commented Aug 4, 2014

This comment was originally written by @jolleekin


I opened a new issue for the JS implementation.

googlearchive/observe-js#64

@jmesserly
Copy link

i think this is fixed, https://github.com/dart-lang/observe/blob/master/lib/src/path_observer.dart#L116

if not it's now possible to pass in a list to PropertyPath

['foo', 'bar', '+baz']


Added AssumedStale label.

@DartBot
Copy link
Author

DartBot commented Feb 4, 2015

This comment was originally written by @jolleekin


It's not fixed though. Take a look at [_setObjectProperty]. This method doesn't check for strings. If you pass a list that contains strings and Symbols, [_setObjectProperty] will fail.

  bool _setObjectProperty(object, property, value) {
    if (object == null) return false;

    if (property is int) {
      if (object is List && property >= 0 && property < object.length) {
        object[property] = value;
        return true;
      }
    } else if (property is Symbol) {
      // Support indexer if available, e.g. Maps or polymer_expressions Scope.
      if (object is Indexable || object is Map && !_MAP_PROPERTIES.contains(property)) {
        object[smoke.symbolToName(property)] = value;
        return true;
      }
      try {
        smoke.write(object, property, value);
        return true;
      } on NoSuchMethodError catch (e, s) {
        if (!smoke.hasNoSuchMethod(object.runtimeType)) rethrow;
      }
    }

    if (_logger.isLoggable(Level.FINER)) {
      _logger.finer("can't set $property in $object");
    }
    return false;
  }

To fix the problem, [_setObjectProperty] needs to handle strings as follows.

  bool _setObjectProperty(object, property, value) {
    if (object == null) return false;

    if (property is int) {
      if (object is List && property >= 0 && property < object.length) {
        object[property] = value;
        return true;
      }
    } else if (property is String) {
      if (object is Indexable || object is Map && !_MAP_PROPERTIES.contains(property)) {
        object[property] = value;
        return true;
      }
    } else if (property is Symbol) {
      try {
        smoke.write(object, property, value);
        return true;
      } on NoSuchMethodError catch (e, s) {
        if (!smoke.hasNoSuchMethod(object.runtimeType)) rethrow;
      }
    }

    if (_logger.isLoggable(Level.FINER)) {
      _logger.finer("can't set $property in $object");
    }
    return false;
  }

@DartBot DartBot added Type-Defect area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. closed-obsolete Closed as the reported issue is no longer relevant labels Feb 4, 2015
@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

This issue has been moved to dart-archive/observe#68.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. closed-obsolete Closed as the reported issue is no longer relevant
Projects
None yet
Development

No branches or pull requests

3 participants