feature request: expose interfaces that other classes can implement about built_collection.dart HOT 13 CLOSED

Thanks! Is the suggested fix to check .runtimeType instead of "is"?

PRs are welcome; I generally get to built_value/built_collection issues quite quickly if they're blocking someone, but non-critical issues wait until I have some spare time, which can take a while :)

from built_collection.dart.

Since a runtimeType check will break for subclasses (class MyBuiltSet extends BuiltSet), I was going to suggest the same approach the quiver authors use for their classes: Provide an abstract class that defines the interface, then provide a default implementation of that interface in a separate class, then add a factory constructor to the interface class that returns an instance of the default implementation.
But now that you mention it, the runtimeType check might be sufficient in this case, because built types must never be subclassed. (if I recall correctly)

Which approach do you prefer?

from built_collection.dart.

Yeah, I don't particularly want to encourage subclasses; if there's something people want to do along those lines I hope they would get in touch :)

So I think checking runtimeType sounds reasonable.

from built_collection.dart.

There's a problem with runtimeType: new BuiltList<int>().runtimeType == BuiltList returns false, because the runtimeType is BuiltList<int>. Both the factory constructor and ListBuilder.replace don't have access to the exact expected type.

I could work around this with
if (iterable.runtimeType == new BuiltList<E>().runtimeType)
but that involves an unnecessary allocation.

Alternatively, I could do (example for BuiltList factory)

try {
  return iterable as BuiltList<E>;
on CastError {
  return new BuiltList<E>._copyAndCheck(iterable);
}

but catching an Error goes against the Dart style guide.

Your thoughts on this?

from built_collection.dart.

Tricky. We could parse runtimeType.toString, but I'm not sure that would be any faster. I'll see if anyone on the languages team has suggestions.

How does it work out if you try to implement those private members? Not hugely pretty, but might be a short term solution.

from built_collection.dart.

How does it work out if you try to implement those private members?

Impossible because protected members are only visible to the same library. If I add a _set property to my class, SetBuilder still can't access it and will throw NoSuchMethodError.

Not hugely pretty, but might be a short term solution.

It's not urgent, I'm probably the only one who will use this package in the near future anyways, and I don't need to create a regular BuiltSet with the contents of my own IndexedSet. I'd just prefer to publish the package when it doesn't have these pitfalls :)

from built_collection.dart.

Ah. Forgot that bit. Heh.

The language team says to go ahead and create a proper interface, as in your original request :) ... I was worried about possible performance implications but apparently there aren't any.

Seems reasonable.

from built_collection.dart.

They also suggested this pattern to avoid having to write the whole public API twice:

abstract class BuiltSometing<T> ... {
   factory BuiltSomething(...) = _BuiltSomething<T>;
   BuiltSomething._(...);
   ... actual implementation ...
}

class _BuiltSomething<T> extends BuiltSomething<T> {
  _BuiltSomething(...) : super._(...);
}

The implementation stays in the public class, but is only ever used concretely in the private subclass.

from built_collection.dart.

OK. I can do that, if you want.

Do you have suggestions/preferences in which files the interfaces should go, and how to rename the existing classes? If not, I'd start with one class, then ask you for a review before I implement the others.

from built_collection.dart.

Sounds good, please do :)

I'd keep BuiltList in the same file, and add _BuiltList as the implementation class in that same file. How does that sound?

Thanks.

from built_collection.dart.

I just remembered that #76 is still open, too. If the default BuiltList becomes a wrapper of arbitrary Lists, should it maybe be called WrappingBuiltList and be part of the public package API? Like this:

abstract class BuiltSometing<T> ... {
   factory BuiltSomething(...) = WrappingBuiltSomething<T>;
   BuiltSomething._(...);
   ... actual implementation ...
}

class WrappingBuiltSomething<T> extends BuiltSomething<T> {
  WrappingBuiltSomething(...) : super._(...);
  WrappingBuiltSomething.using(List<T> base) { ... }
}

from built_collection.dart.

If it's public then we have the same problem that someone might implement it :) ... we can use different factory constructors on BuiltSomething to expose different implementations that might show up later, rather than needing to expose different types.

from built_collection.dart.

Published as 2.0.0. Now to fix built_value :) ... should be able to get a fix out tomorrow.

Thanks!

from built_collection.dart.