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

Could Matcher be changed to be typed as Matcher<T>? #2352

Closed
1 of 3 tasks
matanlurey opened this issue Sep 28, 2016 · 14 comments
Closed
1 of 3 tasks

Could Matcher be changed to be typed as Matcher<T>? #2352

matanlurey opened this issue Sep 28, 2016 · 14 comments
Labels
package:matcher status-blocked Blocked from making progress by another (referenced) issue type-enhancement A request for a change that isn't a bug

Comments

@matanlurey
Copy link
Contributor

matanlurey commented Sep 28, 2016

Specifically, this allows strongly-typed assurances when creating specialized matchers:

abstract class StringMatcher extends Matcher<String> {}

I don't think this is a breaking change, unless we specifically want to allow something like:

expect(5, isNot(equalsIgnoringCase('5')))

Added after the fact by @nex3

Blocking issues (see comments for details):

@jacehensley-wf
Copy link

I would also like to see something like this. In strong mode you cannot override the types of params in methods. Having something like this would be great.

To be able to use the item param, for instance, in a typed manner you have to cast it using as.

class StringMatcher<T extends String> extends Matcher<T> {
  @override
  bool matches(T item, Map matchState) {
    // Use a String method
    if (item.contains('whatever') return true;
  }
}

Vs.

class StringMatcher extends Matcher {
  @override
  bool matches(dynamic _item, Map matchState) {
    item = _item as String;

    // Use a String method
    if (item.contains('whatever') return true;
  }
}

@matanlurey
Copy link
Contributor Author

@jacehensley-wf FWIW, I've been using this pattern for now:

@override
bool matches(item, _) {
  if (item is String) {
    return item.contains('whatever');
  }
  return false;
}

Still not preferable since you get runtime errors, not static errors.

@zoechi
Copy link

zoechi commented Nov 2, 2016

@jacehensley-wf you can now override parameters when you add the @checked annotation https://pub.dartlang.org/packages/meta (1.0.3)

@jacehensley-wf
Copy link

Hmm, I thought I tried that. I'll try it again.

Yeah I am still getting this error:

Invalid override. The type of ClassNameMatcher.matches ((String, Map<dynamic, dynamic>) → bool) is not a subtype of Matcher.matches ((dynamic, Map<dynamic, dynamic>) → bool).

When doing this:

bool matches(@checked String className, Map matchState)

I'm on Dart 1.19.1 using Atom if that matters.

@matanlurey
Copy link
Contributor Author

You might need the latest Dart stable or even development branch.

@jacehensley-wf
Copy link

jacehensley-wf commented Nov 2, 2016

Thanks, I'll try it out on those versions.

Update: Works as expected on Dart 1.20.1!

@nex3 nex3 added type-enhancement A request for a change that isn't a bug status-blocked Blocked from making progress by another (referenced) issue labels Jan 19, 2017
@nex3
Copy link
Member

nex3 commented Jan 19, 2017

I like this idea a lot, especially since strong mode provides a lot more benefits for having nice types everywhere. However, in order to make this useful, we'd need to integrate it into expect(), which would require a few features we don't have yet. In particular:

  • use min/max instead of LUB/GLB in strong mode inference? sdk#27917. Without this, Dart will just infer expect<Object>() for any cases where the matcher doesn't match the value.
  • dart-lang/sdk#4938. The second parameter to expect(), as well as most parameters that take matchers, allow either a matcher or a raw value. In order to have any inference at all, we'd need to be able to express T | Matcher<T>.

@matanlurey
Copy link
Contributor Author

Cool. Stalking those issues :)

@jacehensley-wf
Copy link

@nex3 I think there is still value in adding a generic parameter to Matcher even with out those other issues. I do agree that those with those other issues it would make this much more meaningful.

@zoechi
Copy link

zoechi commented May 17, 2017

Just some update:

@jacehensley-wf you can now override parameters when you add the @checked annotation

@checked was kinda replaced by covariant

@kevmoo
Copy link
Member

kevmoo commented Jun 1, 2017

@nex3 any more thoughts on this?

@nex3
Copy link
Member

nex3 commented Jun 1, 2017

Neither of the blocking issues are fixed, so... no?

@nex3
Copy link
Member

nex3 commented Oct 5, 2017

I believe this is also blocked on dart-lang/sdk/issues/25490, since it requires that type information flow between arguments of expect(). I'm going to edit the original comment and add a list of blocking issues there.

@natebosch
Copy link
Member

Closing this as not planned. This appears to rely on language features we are unlikely to get anytime soon, and we're instead delivering the benefit this would have brought in a new package with a different design. https://pub.dev/packages/checks

@natebosch natebosch closed this as not planned Won't fix, can't repro, duplicate, stale Feb 16, 2023
@mosuem mosuem transferred this issue from dart-lang/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 status-blocked Blocked from making progress by another (referenced) issue type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

7 participants