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

Deprecate isTrue, isFalse #44

Open
natebosch opened this issue May 3, 2017 · 8 comments
Open

Deprecate isTrue, isFalse #44

natebosch opened this issue May 3, 2017 · 8 comments
Labels
type-enhancement A request for a change that isn't a bug
Milestone

Comments

@natebosch
Copy link
Member

These matchers, when used in expect calls, don't add value over literal boolean values.

If there aren't other places which is seeing value from these we should deprecate and eventually remove them so we don't have multiple ways of accomplishing the same thing.

@nex3 nex3 added the type-enhancement A request for a change that isn't a bug label May 16, 2017
@nex3
Copy link
Member

nex3 commented May 16, 2017

I'm of two minds about this. I see where you're coming from, but I also appreciate the aesthetic value of letting matchers read like sentences where possible—"expect foo is true" reads better than "expect foo true".

If we do decide to do this, I'd want to first improve the output of equals() so that we don't have angle brackets for objects like booleans whose string output we know to be reasonable.

@natebosch natebosch added this to the 1.0.0 milestone May 16, 2017
@natebosch
Copy link
Member Author

Added to 1.0.0 milestone so we can either decide not to deprecate, or have them removed before the version bump.

This has come up on mailing lists as a source of confusion, IMO having 1 way to solve this problem outweighs the aesthetics of the call.

It definitely can't hurt to have nicer output from equals() in any case 😄

@matanlurey
Copy link

One advantage of isTrue and isFalse is we could do better static analysis, i.e.:

expect/*Inferred: <bool>*/(someFlag, isTrue);

That means if you accidentally type:

expect('clearlyNotTrue', isTrue);

We could potentially lint this. Just an idea.

@natebosch
Copy link
Member Author

Hmm, yeah with #37 these could potentially have more utility.

Though if we work out the T|Matcher<T> problem that would exist with expect anyway would we also be able to get the same for expect('notABool', true)?

Do we see a world where we start requiring equals() so we can limit the second argument to Matcher?

@nex3
Copy link
Member

nex3 commented May 17, 2017

Do we see a world where we start requiring equals() so we can limit the second argument to Matcher?

I personally use equals() a lot, but I'm in the substantial minority there—people really like using bare values, and I'd hate to take that away from them. Also, even I use bare values for operators where it helps the match read like a sentence, such as expect(foo, isNot(12)). I'd rather just wait for union types.

@jamesderlin
Copy link
Contributor

I'm of two minds about this. I see where you're coming from, but I also appreciate the aesthetic value of letting matchers read like sentences where possible—"expect foo is true" reads better than "expect foo true".

I think that's more about implicit vs. explicit use of equals(). That's orthogonal to isTrue vs. equals(true), which I think is what this issue is really about.

If we do decide to do this, I'd want to first improve the output of equals() so that we don't have angle brackets for objects like booleans whose string output we know to be reasonable.

FWIW, I much prefer the existing output from equals(). I've been confused by the failure output from isTrue/isFalse/isNull where the expected value is printed without angle brackets but the actual value is printed with them. I then spend time wondering why they're formatted differently (e.g. is the actual value a different type?).

@mateusfccp
Copy link

Any plans on this? It always bothered me that we have two ways of accomplishing the same thing, and I myself often use both ways, in an inconsistent manner.

It would be good to, at least, have a lint to improve consistency.

@natebosch
Copy link
Member Author

Any plans on this?

No current plans.

I think this would still need some careful consideration of the costs and benefits. I think there are folks who use these and might feel upset if we deprecate them.

One workaround in the meantime might be to add a CI step that checks for the regex \bisTrue\b or \bisFalse\b since those are unlikely to be used with other meaning in Dart source.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

5 participants