Skip to content
This repository was archived by the owner on Oct 22, 2024. It is now read-only.

fix: Improve the output of many matchers that expect specific types #86

Merged
merged 1 commit into from
Jun 14, 2018
Merged

fix: Improve the output of many matchers that expect specific types #86

merged 1 commit into from
Jun 14, 2018

Conversation

kevmoo
Copy link
Contributor

@kevmoo kevmoo commented Jun 12, 2018 •
edited
Loading

  • Add a new TypedMatcher class to generalize matches type checking
  • Use TypeMatcher across many of the existing Matcher implementations
  • Update tests validate new, more consistent failure messages
  • Add a few new tests for isIn

@kevmoo kevmoo requested review from grouma and natebosch June 12, 2018 21:40
@natebosch
Copy link
Contributor

We had so far made the explicit decision not to do this.

dart-lang/test#2352

I can't tell based on that conversation whether the main consideration is that we can't take advantage of the new types from expect (and don't think they have value outside of that), or whether there is another reason to avoid this.

@kevmoo kevmoo force-pushed the with_generic_matcher branch from fd84382 to 309a3c2 Compare June 12, 2018 23:23
@kevmoo
Copy link
Contributor Author

kevmoo commented Jun 12, 2018 •
edited
Loading

We had so far made the explicit decision not to do this.

In my mind, dart-lang/test#2352 is not about Matcher<T> – it's about expect<T>(T item, expected)

That's tough because expect allows you to pass in anything for expected – there's no way to type the expected param to be T or Matcher<T>

This just makes writing Matcher implementations more easy

@kevmoo kevmoo changed the base branch from lints to master June 13, 2018 02:18
@kevmoo kevmoo force-pushed the with_generic_matcher branch from 309a3c2 to a6a37a8 Compare June 13, 2018 07:54
@kevmoo kevmoo changed the title feature(Matcher): add type param to Matcher for item in functions fix: improve handling of failures due to type mismatches Jun 13, 2018
@kevmoo kevmoo changed the title fix: improve handling of failures due to type mismatches fix: Improve the output of many matchers that expect specific types Jun 13, 2018
@kevmoo
Copy link
Contributor Author

kevmoo commented Jun 13, 2018

Took a different approach, @natebosch . PTAL

@kevmoo kevmoo force-pushed the with_generic_matcher branch from a6a37a8 to 7f1469a Compare June 13, 2018 08:00
@kevmoo kevmoo requested a review from jakemac53 June 13, 2018 14:28
@jakemac53 jakemac53 removed their request for review June 13, 2018 14:50
@jakemac53
Copy link
Contributor

going to let Nate do this one I don't think we need 3 people to review this, unless there is something specific...

@kevmoo
Copy link
Contributor Author

kevmoo commented Jun 13, 2018

Thoughts, @natebosch ?

@natebosch
Copy link
Contributor

There's a lot to consider - can we discuss in person sometime soon?

@kevmoo
Copy link
Contributor Author

kevmoo commented Jun 13, 2018 via email

@kevmoo kevmoo mentioned this pull request Jun 13, 2018
Merged
@kevmoo kevmoo force-pushed the with_generic_matcher branch from 7f1469a to 83b0d4b Compare June 13, 2018 22:16
@kevmoo kevmoo changed the base branch from master to add_check_type_matcher June 13, 2018 22:17
@kevmoo kevmoo force-pushed the add_check_type_matcher branch from 8c42876 to e4c7d12 Compare June 13, 2018 22:46
@kevmoo kevmoo force-pushed the with_generic_matcher branch 2 times, most recently from 09a7cb1 to d1bcd54 Compare June 14, 2018 00:56
@kevmoo kevmoo force-pushed the add_check_type_matcher branch 2 times, most recently from 4ff649f to 2fc1ab2 Compare June 14, 2018 18:34
@kevmoo kevmoo force-pushed the with_generic_matcher branch from d1bcd54 to f81b17f Compare June 14, 2018 18:39
@kevmoo kevmoo force-pushed the add_check_type_matcher branch from 2fc1ab2 to 52a53e9 Compare June 14, 2018 20:19
@kevmoo
Copy link
Contributor Author

kevmoo commented Jun 14, 2018

@natebosch – and finally...

@kevmoo
fix: Improve the output of many matchers that expect specific types
7ca775b 
- Add a package-private FeatureMatcher class to generalize type checking
- Use it across many of the existing Matcher implementations
- Update tests validate new, more consistent failure messages
- Add a few new tests for `isIn`
@kevmoo kevmoo force-pushed the with_generic_matcher branch from f81b17f to 7ca775b Compare June 14, 2018 20:22
@kevmoo kevmoo changed the base branch from add_check_type_matcher to master June 14, 2018 20:22
natebosch
natebosch reviewed Jun 14, 2018
View reviewed changes
test/core_matchers_test.dart
if (isDart2) {
// With Dart2 semantics, predicate picks up a type argument of `bool`
// and we get nice type checking.
// Without Dart2 semantics a gnarly type error is thrown.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we going to be prevented from publishing this until Dart 2 is the default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope! This test is just skipped w/ Dart2. All tests pass with and without

@kevmoo kevmoo merged commit 98bd8b5 into master Jun 14, 2018
@kevmoo kevmoo deleted the with_generic_matcher branch June 14, 2018 20:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@natebosch natebosch natebosch approved these changes

@grouma grouma Awaiting requested review from grouma

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kevmoo @natebosch @jakemac53 @googlebot