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

Matcher.matches should return a rich "match result" object #2325

Open
seaneagan opened this issue Apr 1, 2015 · 2 comments
Open

Matcher.matches should return a rich "match result" object #2325

seaneagan opened this issue Apr 1, 2015 · 2 comments
Labels
package:matcher type-enhancement A request for a change that isn't a bug
Milestone

Comments

@seaneagan
Copy link
Contributor

Currently Matcher.matches just returns bool, and the rest of the match result is recorded separately by mutating an untyped Map matchState object that must be initialized and passed in. That's pretty strange. Instead it should just return the whole match result as a single immutable object.

class Matcher {
  Description describe();

  /// Returns either [MatchResult] of [Future<MatchResult>].
  matches(item);
}

abstract class MatchResult {
  bool get isMatch;
}

class Match {
  Match();
  bool get isMatch => true;
}

abstract class Mismatch extends MatchResult {
  bool get isMatch => false;
  Description describeMismatch({bool verbose: false});
}

Subclasses of Mismatch can add typed (likely private) fields for use by describeMisMatch, and a constructor to initialize them.

In the case when Matcher.matches returns a Future, expect from the test package would also return a Future which could be awaited. Alternatively there could a separate predict method for the async case. The ability to return a Future would allow for matchers which provide nicer error messages for things like testing asynchronous things like http calls. Also some of the existing APIs like completes and completion could use this, which would then allow them to finish executing before the test continues on to the next expectation:

  await predict(future, completion(5));
  expect(foo, bar);

edit: Renamed Matched to Match since dart:core Match will automatically be shadowed anyways.

@seaneagan
Copy link
Contributor Author

Also, I'd prefer the name Matcher.match for this. Which now that I think about it, maybe that would be a way to do this in a more backward compatible manner, the default implementation of Matcher.match could be implemented in terms of the old matches behavior!

@nex3 nex3 added the type-enhancement A request for a change that isn't a bug label Apr 1, 2015
@nex3 nex3 added this to the milestone Apr 1, 2015
@nex3
Copy link
Member

nex3 commented Apr 1, 2015

I like this idea, with a few caveats. First, I don't think anything in matcher should be asynchronous. Keeping the asynchrony in unittest makes it a lot easier to add test-specific tracking to asynchronous calls, and putting it in matcher means that every matcher that deals with other matchers has to be resilient them possibly being async. Second, we can't use the name Match. Shadowing core library types can produce very confusing behavior that I'd rather not subject users to.

I'm not too worried about backwards-compatibility for people defining their own matchers. With all the changes that need to happen to make the API nicer, I suspect they're going to get broken one way or another. As long as people using matchers don't break too badly, I'm happy.

@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