rename `t.same()` to `t.deepEqual()` #686

kentcdodds · 2016-03-27T03:52:54Z

Do not merge :-)

This is a work in progress PR just to get the conversation started. There are several tests that are failing and I could use some help there I think.

Thoughts?

mention-bot · 2016-03-27T03:52:54Z

By analyzing the blame information on this pull request, we identified @kasperlewau, @ariporad and @sotojuan to be potential reviewers

kentcdodds · 2016-03-27T03:53:47Z

lib/assert.js

+	return x.notDeepEqual(val, expected, msg);
+};
+
+function deprecationNotice(oldApi, newApi) {


I expect we'll want something a little more robust :-)

kentcdodds · 2016-03-27T03:54:44Z

And shall I just say that because the tests for this project are written in tap rather than AVA, I'm coming to appreciate AVA more and more :-) ❤️

sindresorhus · 2016-03-27T18:06:27Z

Thanks for starting the conversation @kentcdodds :)

We'll definitely need a codemod if we're going with this.

sindresorhus · 2016-03-27T18:25:39Z

Not to start a bikeshedding, but is there any other word we could use that might be even clearer?

kentcdodds · 2016-03-27T18:26:38Z

Great to explore options for the best API possible while the topic is open for debate. I have no ideas though...

vadimdemedes · 2016-03-27T18:43:11Z

Here's an idea: what if t.same becomes t.is? Couldn't be clearer and shorter to type. And t.is gets removed in favor of t.true(a === b).

I use t.is only for primitive values and when I need to check that references are equal. t.same also handles primitive values, so the only benefit of t.is is checking reference equality.

What do you think?

kentcdodds · 2016-03-27T20:51:36Z

I'd be fine with that.

jamestalmage · 2016-03-27T20:51:58Z

I think the best criteria by which to judge is this:

If someone who has never used tap/AVA/chai/etc is reading the test, what gives them the best chance of understanding what the assertion does without looking at the docs. Part of this includes avoiding ambiguity where possible. If a word could mean two different types of assertions, that's a big knock against it (we won't reach perfection here, because English).

Short names should be low on the list of priorities. There is a limit at which something is too long, but a few extra characters is worth any added clarity.

kentcdodds · 2016-03-27T20:55:32Z

Short names should be low on the list of priorities. There is a limit at which something is too long, but a few extra characters is worth any added clarity.

Couldn't agree more.

But I still have no suggestions to a better name than deepEqual :-)

kasperlewau · 2016-03-27T20:59:43Z

Not to start a bikeshedding, but is there any other word we could use that might be even clearer?

Off of the top of my head, here's a couple of candidates (of varying degrees of suck);

t.sameStructure
t.distinctEq
t.reflect
- t.reflection
- t.reflectionOf
t.mirror
- t.mirrorOf
t.comparable
- t.comparableTo
t.represent
- t.representation
- t.representationOf
t.isomorphism
- t.isomorphicTo

The ones that I do like to some extent would be sameStructure and comparableTo. Possibly distinctEq as well, even though it's somewhat of an oxymoron. Can probably bin the rest swiftly 😄

I reckon sameStructure is the winner here. I find it very hard to misinterpret, and it conveys the intent of the assertion very clearly. I don't quite see it trumping deepEqual though. Just gotta keep crossing off those candidates I suppose..

Here's an idea: what if t.same becomes t.is? Couldn't be clearer and shorter to type. And t.is gets removed in favor of t.true(a === b).

I agree, in parts. It's ridiculously short to type, and I think opting for t.true(a === b) is a good way to go so there's that going for it. But I don't think t.is adequately conveys the intention of comparing two distinct objects of the same structure.

I'm sorry to say I don't have any better suggestions, aside from the list of verbose candidates in my first segment - along with deepEqual.

My dream scenario would be for the deepEquality assertion to die off entirely and we'd have something like t.true(Object.equals(a,b))

In all honesty, I find deepEqual hard to beat - purely based on the fact that it's "widely adopted". It's probably something most of us are already familiar with. If not, it's pretty easy to find out with a quick Google search as it's not widely adopted outside of computer programming from what I can find. same, is, and most of my suggestions above - are and could be.

jamestalmage · 2016-03-27T21:10:49Z

My concern with t.is is what happens to users who fail to update current t.is assertions. We leave them open to subtle bugs. Also, it doesn't pass the disambiguation test.

novemberborn · 2016-03-28T10:47:47Z

Regardless which name we pick we should carefully document which values are considered passing and which aren't. E.g. #630 surfaced that we were requiring constructor equality.

I wonder if the "same" or "equal" part is implicit. You're testing an assertion with an actual and an expected value. t.same() is confusing because obviously you want things to be the same… and then what does t.is() mean?

Given that, how about t.deep()?

novemberborn · 2016-03-28T10:49:48Z

Which would lead to t.strict() for t.is(), if we want to go that far.

kentcdodds · 2016-03-28T21:11:22Z

how about t.deep()?

I still prefer deepEqual. To me it's confusing without equal.

sindresorhus · 2016-03-30T05:02:34Z

I'm good with deepEqual.

We should really have a codemod for this change before releasing it. Anyone interested in trying to make one (I especially encourage community members to try)? Resources: #644

jamestalmage · 2016-04-01T18:16:28Z

Anyone interested in trying to make one (I especially encourage community members to try)?

Looping in our AST friends: @twada @jfmengels

spudly · 2016-04-01T18:34:38Z

Whipped up a quick codemod for this: https://astexplorer.net/#/AbMVMKHZhp

I'm happy to put this in a PR. Where should I put the files? Perhaps in /codemods?

jfmengels · 2016-04-01T22:47:16Z

@spudly 👍!

I'd create a new project containing the codemods (ava-codemods?). Don't see the value of putting it in the AVA project if it's only going to be run once at the very most.
There is talk about writing other (complex) codemods too in #644.

jfmengels · 2016-04-01T22:54:17Z

@spudly I improved your code a bit by providing support for notSame --> notDeepEqual (and some necessary refactoring to avoid duplicating) https://astexplorer.net/#/yXo9G9wx9m/1

spudly · 2016-04-01T23:16:02Z

@sindresorhus, @jamestalmage, @vdemedes, @novemberborn

Opinions about where to put codemods? I was thinking in /codemods but @jfmengels recommended a separate repo. I'll do whatever.

spudly · 2016-04-01T23:17:58Z

@jfmengels,

Looks good and nice refactor. Guess I forgot about notSame/notDeepEqual because I never use them...

sindresorhus · 2016-04-03T19:17:14Z

See https://github.com/jamestalmage/ava-codemods.

sindresorhus · 2016-04-04T16:45:55Z

lib/assert.js

+};
+
+function deprecationNotice(oldApi, newApi) {
+	console.warn(`DEPRECATION NOTICE: ${oldApi} has been renamed to ${newApi} and will eventually be removed`);


We should mention the codemods here or at least a link to docs that mention it.

Definitely. Would you like a more robust deprecation abstraction, or is this simple function and console.warn sufficient?

You could use util.deprecate.

sindresorhus · 2016-04-04T16:49:16Z

@kentcdodds We now have a codemod, so this could potentially go into the next release. Would you be able to finish this up and fix the failing tests?

kentcdodds · 2016-04-04T16:49:32Z

Could definitely use some help on this to know why tests are failing. It's confusing with mix of ava and assert :-)

I definitely want to make this happen soon! :D

kentcdodds · 2016-04-04T17:43:36Z

Updated the deprecation to use util.deprecate (TIL) and added a note about the codemod repo. I'll try to give the failing tests another look, but last time I know I had a bit of trouble with them.

kentcdodds · 2016-04-04T19:02:42Z

test/assert.js

+test('.same() should log a deprecation notice', function (t) {
+	var calledWith;
+	var originalConsole = console.warn;
+	console.warn = function () {


Turns out that when using util.deprecate, it actually logs to console.log which is fine, but I can't seem to get this test to work because when I reassign console.log it appears that my reassignment is never called. I'm not sure what's going on here... Any tips appreciated.

Alternatively, we can trust that util.deprecate works and not worry about writing tests for this case... ¯_(ツ)_/¯

Looks like deprecate calls an undocumented method process.emitWarning.

That ends up causing a warning event to be emitted on process, which seems to be handled by writing to console.error.

So try console.error?

Or just trust that util.deprecate works.

Or just trust that util.deprecate works.

If everyone's ok with that, I think that's what I wanna do :-)

Node.js core uses util.deprecate. So I would trust it.

kentcdodds · 2016-04-04T19:24:13Z

Alrighty, this should be ready to go I think. Feedback welcome!

kentcdodds · 2016-04-04T19:24:32Z

lib/assert.js

@@ -130,8 +130,6 @@ x.notThrows = function (fn, msg) {
 	}
 };

-x.doesNotThrow = util.deprecate(x.notThrows, 't.doesNotThrow is renamed to t.notThrows. The old name still works, but will be removed in AVA 1.0.0. Update your references.');


Moved this down to the other deprecated APIs

kentcdodds · 2016-04-04T19:26:17Z

lib/enhance-assert.js

-	't.same(value, expected, [message])',
-	't.notSame(value, expected, [message])',
+	't.deepEqual(value, expected, [message])',
+	't.notDeepEqual(value, expected, [message])',


This bit made things tricky for me in test/test.js which asserts t.is(result.result.assertCount, 1); and there was no real way for me to know that this is what causes assertCount to increment. Still not sure I follow but this fixed it so ¯_(ツ)_/¯

The power-assert enhancement actually has callbacks (onSuccess, onFailure) that are called with the result of each assertion. That is where we implement assertion tracking.

I think you should probably keep t.same and t.notSame in the patterns until they are officially removed. Otherwise this will break assertionCount for them. Maybe add a test that ensures the assertion count is tracked for those (which we will remove when we drop support).

kentcdodds · 2016-04-04T19:28:09Z

Also, let me know if you want me to squash and clean things up with the commit message or if you're fine using GitHub's fancy new squash and merge feature :-)

jamestalmage · 2016-04-04T19:43:13Z

We will use GH' squash and merge.

jamestalmage · 2016-04-04T19:45:04Z

lib/assert.js

+x.notSame = util.deprecate(x.notDeepEqual, getDeprecationNotice('notSame()', 'notDeepEqual()'));
+
+function getDeprecationNotice(oldApi, newApi) {
+	return `DEPRECATION NOTICE: ${oldApi} has been renamed to ${newApi} and will eventually be removed. See https://github.com/jamestalmage/ava-codemods to help upgrade your codebase automatically.`;


We don't transpile AVA. Can't use string templates on Node 0.10

Ah, good to know. Will update.

jamestalmage · 2016-04-04T19:48:20Z

A few notes, mostly related to making sure we are deprecating t.same without breaking it.

Once those are cleared up, LGTM.

kentcdodds · 2016-04-04T19:49:19Z

Updated PR to respond to @jamestalmage's comments. Thanks!

jamestalmage · 2016-04-04T19:56:38Z

index.d.ts

+ 	 */
+ 	same<U>(value: U, expected: U, message?: string): void;
+ 	/**
+ 	 * Assert that value is not deep equal to expected. (DEPRECATED use `notDeepEqual`)


Move the DEPRECATED warning to the start?

IDE's use these comments for tooltips. So it would be good to have the warning as prominent as possible.

jamestalmage · 2016-04-04T19:57:27Z

LGTM.

novemberborn · 2016-04-05T10:21:43Z

!

sindresorhus · 2016-04-05T10:27:26Z

Yay for better UX. Thanks for nudging us towards this and doing the work @kentcdodds :)

kentcdodds · 2016-04-05T13:25:29Z

Fantastic! Did we decided on doing truthy/falsy? I'm happy to do that work as well

sindresorhus · 2016-04-05T13:46:58Z

Yes. We're going with that. PR super appriciated :)

avajs/ava#686 avajs/ava#716

kentcdodds reviewed Mar 27, 2016
View reviewed changes

sindresorhus added the DO NOT MERGE label Mar 27, 2016

sindresorhus mentioned this pull request Mar 28, 2016

prevent start if already started - fixes #5 sindresorhus/ora#6

Closed

sindresorhus reviewed Apr 4, 2016
View reviewed changes

sindresorhus changed the title ~~WIP: rename same to deepEqual~~ rename t.same() to t.deepEqual() Apr 4, 2016

sindresorhus removed the DO NOT MERGE label Apr 4, 2016

Kent C. Dodds added 2 commits April 4, 2016 11:37

WIP: rename same to deepEqual

17a1e9d

Update deprecation method to use util.deprecate

ef49c0f

kentcdodds force-pushed the pr/same-to-deepEqual branch from df2d1d0 to ef49c0f Compare April 4, 2016 17:42

kentcdodds reviewed Apr 4, 2016
View reviewed changes

Finish fixing all tests

55cf5c9

kentcdodds reviewed Apr 4, 2016
View reviewed changes

--amend

e9a2b2a

kentcdodds reviewed Apr 4, 2016
View reviewed changes

jamestalmage reviewed Apr 4, 2016
View reviewed changes

Responding to PR comments

4a804f7

kentcdodds force-pushed the pr/same-to-deepEqual branch from b9e601c to 4a804f7 Compare April 4, 2016 19:50

jamestalmage reviewed Apr 4, 2016
View reviewed changes

Move DEPRECATED in .d.ts

4c15c4f

sindresorhus merged commit e9c6cc2 into avajs:master Apr 5, 2016

kentcdodds deleted the pr/same-to-deepEqual branch April 5, 2016 14:06

sindresorhus added a commit to avajs/atom-ava that referenced this pull request Apr 5, 2016

update snippets per our assert name changes

333a623

avajs/ava#686 avajs/ava#716

sindresorhus added a commit to avajs/sublime-ava that referenced this pull request Apr 5, 2016

update snippets per our assert name changes

fc0773b

avajs/ava#686 avajs/ava#716

rename t.same() to t.deepEqual() #686

rename t.same() to t.deepEqual() #686

Conversation

kentcdodds commented Mar 27, 2016

mention-bot commented Mar 27, 2016

Choose a reason for hiding this comment

kentcdodds commented Mar 27, 2016

sindresorhus commented Mar 27, 2016

sindresorhus commented Mar 27, 2016

kentcdodds commented Mar 27, 2016

vadimdemedes commented Mar 27, 2016

kentcdodds commented Mar 27, 2016

jamestalmage commented Mar 27, 2016

kentcdodds commented Mar 27, 2016

kasperlewau commented Mar 27, 2016

jamestalmage commented Mar 27, 2016

novemberborn commented Mar 28, 2016

novemberborn commented Mar 28, 2016

kentcdodds commented Mar 28, 2016

sindresorhus commented Mar 30, 2016

jamestalmage commented Apr 1, 2016

spudly commented Apr 1, 2016

jfmengels commented Apr 1, 2016

jfmengels commented Apr 1, 2016

spudly commented Apr 1, 2016

spudly commented Apr 1, 2016

sindresorhus commented Apr 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Apr 4, 2016

kentcdodds commented Apr 4, 2016

kentcdodds commented Apr 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds commented Apr 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds commented Apr 4, 2016

jamestalmage commented Apr 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamestalmage commented Apr 4, 2016

kentcdodds commented Apr 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamestalmage commented Apr 4, 2016

novemberborn commented Apr 5, 2016

sindresorhus commented Apr 5, 2016

kentcdodds commented Apr 5, 2016

sindresorhus commented Apr 5, 2016

rename `t.same()` to `t.deepEqual()` #686

rename `t.same()` to `t.deepEqual()` #686