feature: Upgrade TypeMatcher, deprecate isInstanceOf

[kevmoo]

TypeMatcher

• No longer abstract
• Added type argument
• Deprecate the existing name parameter, tell folks to the type param
• Added having method which allows chained validation of features
• Eliminated 13 private implementations from the package
  • Just use it directly.
• Moved to its own file

Deprecate isInstanceOf class.

• Tell folks to use TypeMatcher<T> instead
• Run away from weirdly named classes

Tests

• centralizing tests in type_matcher_test
• Removed isInstanceOf tests from core_matchers_test

[kevmoo]

Very much in the spirit of #86

[kevmoo]

@natebosch – now this is ready for review!

[kevmoo]

@natebosch only a handful of breaks in pkg:test tests – due to expected strings in one file. Easy find-replace to fix

[Hixie]

This broke a bunch of stuff in Flutter. Can we revert it and/or talk about why this is a good idea?

[matanlurey]

Did it actually break something or are you failing on deprecation warnings? @Hixie

[kevmoo]

If this is all deprecation warnings, we're not going to revert. All existing functionality is intact. We cannot hold back responsible API evolution (with proper deprecations, etc) because users have strict analysis checks.

We've gone through a number of our packages and fixed up the usage. This is progress.

[Hixie]

It broke things because switching to TypeMatcher conflicts with our own TypeMatcher.

It also is less idiomatic.

   expect(foo, isInstanceOf<Foo>());

...reads nicely.

   expect(foo, TypeMatcher<Foo>());

...makes no sense, in the expect style where the matcher is a verb clause. It sounds like it means that foo types "matcher foo", which is nonsense.

Also, are you never going to remove it? If you're ever going to remove it, then that's a backwards-incompatible change, and that's what I object to. If you're never going to remove it then why deprecate it?

Our users write tests and we can't just be willy-nilly breaking them without good reason. What's the good reason for removing isInstanceOf?

[kevmoo]

First, I was a bit harsh in my first reply. My apologies. Breaks from "dot" releases suck. You guys are trying to move fast. I want to help you.

Conflicting type names – that's a painful edge of the breaking change rule. If adding a class (or a member, if you worry about subclassing) was considered a breaking change, we'd have a bigger mess.

I'm not sure there is anything we can do there. Do we create some sort of package try bot to validate API updates for frequently used packages before changing APIs? Might be a good idea.

RE isInstanceOf – I think we should remove it. Or make it a method. Having a class named isInstanceOf is very much against our style guide.

I'm certainly open to doing a major release of pkg:matcher (which means a major release of pkg:test) where isInstanceOf is turned into a function – although then I'd rather have isA<T>

[Hixie]

I'd be fine with turning it into a function; that would be a transparent change assuming that people have already removed the "new". It's when it gets renamed that there's a breaking change.

[Hixie]

I've submitted a PR to revert us to the 0.12.2 version for now (flutter/flutter#18614), on the assumption that matcher will eventually just export isInstanceOf as a function. Right now tests that use flutter_test can do so (I've exported an isInstanceOf function and hidden the class), and tests that use test directly are still saying const isInstanceOf since we have a lint that requires the const.

[natebosch]

It's when it gets renamed that there's a breaking change

Nothing was renamed. A new class was added and a class was deprecated. The breakage from conflicting imports is an edge of the Dart semver approach that has some sharp edges like this, but we do not think it's feasible to restrict ourselves from ever adding new symbols to the top level namespace for package:test

I've submitted a PR to revert us to the 0.12.2 version for now

You'll still want to clean up the places were we conflict with TypeMatcher. We don't have intentions of removing that class since it would be breaking for all the places that use it now that it's release and we consider type removal breakages to be higher consequence than adding a symbol.

on the assumption that matcher will eventually just export isInstanceOf as a function

This is very unlikely to happen and would be breaking for lots of code. If anything we will consider adding the top level method TypeMatcher<T> isA<T>() for more readable expects.

Right now tests that use flutter_test can do so (I've exported an isInstanceOf function and hidden the class)

Did you do this with a breaking version bump? Is there guaranteed to be no code still using new, or implements or extends against this symbol?

[natebosch]

I will note that if you want flutter_test to be more strict and use a breaking version bump any time a new class or top-level method is added you could add an explicit show to your export and do a major version bump each time you add something to it.

[Hixie]

Is there guaranteed to be no code still using new, or implements or extends against this symbol?

Code using new is likely to exist; however, we are moving our ecosystem towards not having new, so our users are actually ok with that particular change. We're not aware of anyone extending or implementing isInstanceOf. Doing so would be quite strange.