-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
add truthy/falsy alias #663
Conversation
By analyzing the blame information on this pull request, we identified @kasperlewau, @sotojuan and @SamVerschueren to be potential reviewers |
Sorry, here's an example of my use case: test('passes with a relative path', t => {
const output = {filename: relativePath}
const result = validate(output.filename)
t.notOk(result)
})
test('returns error if given an absolute path', t => {
const output = {filename: absolutePath}
const result = validate(output.filename)
t.ok(result)
}) In this scenario, it "passes" if it's |
ce6a8a4
to
a52ffd0
Compare
Build broke because of linting. Sorry, I'm a creature of habit (http://kcd.im/semicolons) :-) I've force pushed a fix. |
We have intentionally not added any aliases (there are a lot of them in @vdemedes @novemberborn @jamestalmage Thoughts? |
Totally understand the value of avoiding aliases and respect if it's decided to avoid them 👍 |
a52ffd0
to
7f8c7f4
Compare
Opened another PR to add a note to the |
I wish the functions were called If we were to alias them, I would want to immediately deprecate Truth be told, I wish a number of our assertion methods were named differently (i.e. It is not possible for you to know what Fact of the matter is, it took me just a few hours to commit AVA's limited list of assertions to memory, and now when I look at other peoples AVA tests I know what they mean without pulling up the docs. Also, I think there is value in supporting custom assertions someday, and every alias we add removes another name from the namespace. |
I realize that there's a community with code that depends on these things, but I think that as a pre-1.0.0 project, there's a responsibility to get the API correct and obvious, even at the cost of breaking changes. If it's really that bad, we could write a codemod that people could run on their code.
I think this is awesome. It would be even more awesome if learning what these assertions do doesn't require a docs lookup. I feel like I'd be happy to make the PR to alias things and add a deprecation message. |
@kentcdodds - I personally like that plan. But I would wait for other maintainers to chime in before proceeding. The codemod would be a must IMO. Maybe we could even ship the codemod with AVA for a while and offer a CLI prompt to apply it automatically. We already know which files are the test files from the config, so we could make it pretty easy for users. (I think deciding whether or not to ship it with AVA depends on how many dependencies the codemod would add). |
Major +1 on promoting power-assert. If only I could do
"Okay" doesn't feel like an absolutist assertion. Seems on-par with truthy/falsy (and I never know how to spell falsey). Similarly
Would be good to have a codemod for breaking changes like this, but let's keep it as a separate package. Could implement a stub assertion that fails the test with a message pointing towards the codemod. If we ever get to that point. |
You can google My problem with |
I've had the same experience. I don't want to bikeshed on stuff, and (so far) it looks like this is pretty subjective anyway :-) |
Not opposed to changing this but I don't necessarily feel the need either ¯_(ツ)_/¯ |
I think |
Personally, I'm fine with As I see, our opinions on this are separated, so would be good to get more input from AVA users. /cc @sotojuan @twada @floatdrop @ariporad @kasperlewau @BarryThePenguin @jokeyrhyme @naptowncode @spudly |
If we're going to get people's opinions (this is great). I propose that we just use GitHub issue reactions to do a voting system. 👍 this comment if you're in favor of changing 👎 this comment if you'd prefer to keep 😕 if you don't care either way. |
I'm with you but I think I am biased— That said, I think If we do make a change I think it'd be better to soft deprecate for now till 1.0 ( |
FWIW, here are the results
For me this shouldn't make a difference. Readability/maintainability should go before how many characters there are IMO
👍 |
I wasn't going to spam everyone with a +1 (no need these days), but since I was specifically invited...
I've always hated I'm all for adding |
... also if the recommended is always going to be to lean on powerassert, then I think that |
|
I take this back. I've been thinking a lot about this because I'm refactoring tons of mocha tests to be ava tests and I've decided that I was wrong. I think instead of adding truthy/falsy, we should just replace all of the assertion functions with one single function that fails the test if the provided value is falsy. The following would all be deprecated (recommended replacement on the right):
We'd still have to have |
I'll chime in with my some thoughts on the subject. I try to stay away from
t.true(obj.hasOwnProperty('foo'));
t.true('foo' in obj);
t.true(fn() !== undefined); What's of importance to me when writing unit tests, is for my peers and coworkers to be able to look at the tests and just get it at first glance, without having to sift through docs. A lot of us are working on pet projects outside of our daily jobs, and the amount of API surface that you have to get comfortable with nowadays is quite vast. I think the smaller the API surface of Kill' em all I say - I don't see myself using either
I'm in full agreement here. I really didn't grok Not wanting to introduce aliases, I'd support a |
IMO, the value of
|
Yeah. I kinda regret that one now. It sounded clear when we added it.
But if neither is clear enough by the code itself, it's doesn't really matter if one is more Google-able. The documentation would cover either. |
truthy vs ok: I would argue you have at least a chance of understanding the intent of the If I was describing some JS algorithm in English, I would never use the phrase "If the returned value is So, that is my best argument for same vs deepEqual: I do like Ultimately, I am willing to live with whatever we decide, but I do think this line from @kentcdodds bears repeating:
|
I suppose there's some merit to the notion that The issue as I see it regarding If you wanted to be really explicit about it and communicate a (to me) crystal clear intent (which I think trumps character count any day of the week when it comes to writing tests), It's not something I've ever come across before, and there's probably good reasons for that - but I think it does tick off some boxes:
I'll end this comment off by saying I'm happy the discussion is alive and kicking, but would it make sense to move it out of the PR? It feels as though we're drifting too far out into off topic land and not discussing the code at hand. Sorry about that. |
Meh. I think @kentcdodds was really after a discussion more than getting this merged. |
Fair enough :) |
If you all recall, this is the precise reason that I created this in the first place:
The intuition of an API is hard to get right for all use cases, but I think that we can all agree that there are more cases where On
Yes, this is true. But I think we've reached a point where we need to start seeing and talking about code (even if it's not quite agreed upon yet, the PR doesn't have to be merged 😄). Either way, this PR will not be merged, so I'll go ahead and close it :-) |
@@ -30,6 +30,8 @@ test('.ok()', function (t) { | |||
assert.ok(true); | |||
}); | |||
|
|||
t.same(assert.ok, assert.truthy); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed that even in this very PR it is evident that I confused .same()
with .is()
:-)
Well that convinced me. +1 for changing to |
Another approach might be to drop / deprecate all truthy / falsey behaviour and encourage developers to use strictly boolean expressions. |
I'd be fine with that as long as we provide a list of good utility libraries for doing common test cases (like |
I think those batteries should come included. |
Even better |
Hence |
I'd be sad to see @novemberborn now I'm convinced we should migrate to |
Use the editor snippets. They autocomplete ;) |
Happy to make any changes you'd like. I'd just like to get the discussion going about this :D