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

Improve matcher isInstanceOf now that Dart has first class types. #2334

Closed
DartBot opened this issue Jun 5, 2015 · 9 comments
Closed

Improve matcher isInstanceOf now that Dart has first class types. #2334

DartBot opened this issue Jun 5, 2015 · 9 comments
Labels
package:matcher type-enhancement A request for a change that isn't a bug

Comments

@DartBot
Copy link

DartBot commented Jun 5, 2015

Originally opened as dart-lang/sdk#15381

This issue was originally filed by greg...@gmail.com


Current: new isInstanceOf<T>(T t)

New: isInstanceOf2(Type t). // But with a better name.

Consider adding:
  throwsType(Type t)

Also worth updating these docs:

"As types are not first class objects in Dart we can only approximate this test by using a generic wrapper class."

"Note that this does not currently work in dart2js; it will match any type, and isNot(new isInstanceof<T>()) will always fail. This is because dart2js currently ignores template type parameters."

https://api.dartlang.org/docs/channels/stable/latest/matcher/isInstanceOf.html

https://www.dartlang.org/articles/writing-unit-tests-for-pub-packages/

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/5479?v=3" align="left" width="48" height="48"hspace="10"> Comment by sethladd


Removed Type-Defect label.
Added Type-Enhancement, Area-UnitTest, Triaged labels.

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/17034?v=3" align="left" width="48" height="48"hspace="10"> Comment by kevmoo


Removed Area-UnitTest label.
Added Area-Pkg, Pkg-Unittest labels.

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/17034?v=3" align="left" width="48" height="48"hspace="10"> Comment by kevmoo


Removed Pkg-Unittest label.
Added Pkg-matcher label.

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/248818?v=3" align="left" width="48" height="48"hspace="10"> Comment by vicb


This is something I have implemented in Guinness as:

    bool matches(obj, Map matchState) =>
        mirrors.reflect(obj).type.isSubtypeOf(mirrors.reflectClass(_type));

see https://github.com/vsavkin/guinness/blob/5f9e291f9c0937a19ab278b0afe7c377bfae25fe/lib/src/common/unittest_backend.dart#L191 for the full Matcher

We have started to discuss this with Kevin: https://plus.google.com/u/0/+VictorBerchet/posts/bdDgAfUB9bW?cfem=1

I'm not clear on how this would affect the generated code size:

  • we don't need more info as what the current impl (isInstanceOf<T>(T t)) uses, would there be a way to prevent the JS size to sky rocket ?
  • even if there is no way to limit the JS size, would that be a pb - considering we are running tests ?

(I've not really considered JS & its code size for my use case. The reasoning being that if the tests pass with Dart, they should pass after dart2js, if not then this is a dart2js and not your own code issue. The tests are supposed to test your own code, not dart2js - still I'm all for testing with dart2js to help stabilize the tools as long as there is no constraint on my development flow).

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

This comment was originally written by greg...@gmail.com


I was under the impression that this was already possible:

  bool isInstance(obj, Type type) => obj is type;
  
But it looks like this is possible - yet?

"malformed type: line 5 pos 18: using 't' in this context is invalid"

I wonder if there is a bug for implementing this - or if it is not possible. In the VM it must just be an int check. Not sure about dart2js.

@DartBot DartBot added the type-enhancement A request for a change that isn't a bug label Jun 5, 2015
@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

This comment was originally written by greg.lo...@gmail.com


Erm - "But it looks like this is NOT possible - yet?"

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/444270?v=3" align="left" width="48" height="48"hspace="10"> Comment by seaneagan


In issue dart-lang/sdk#10406 gbracha indicates that this will only be possible via mirrors. Matchers that use mirrors are currently put here:

http://www.dartdocs.org/documentation/matcher/0.11.1/index.html#matcher/matcher-mirror_matchers

Also this issue is a duplicate of issue #214 which was wrongly merged with issue dart-lang/sdk#8138.

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/444270?v=3" align="left" width="48" height="48"hspace="10"> Comment by seaneagan


Once this is fixed, all the custom throwsArgumentError etc. matchers could easily be removed / cleaned up, since you could just do throwsA(ArgumentError).

@nex3
Copy link
Member

nex3 commented Jun 7, 2017

I believe there are no plans to allow is to work with first-class types, so this isn't feasible.

@nex3 nex3 closed this as completed Jun 7, 2017
@mosuem mosuem transferred this issue from dart-archive/matcher Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:matcher type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

3 participants