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

Default to deep copy #68

Closed
hueniverse opened this issue Apr 30, 2016 · 12 comments
Assignees
Milestone

Comments

@hueniverse
Copy link
Member

@hueniverse hueniverse commented Apr 30, 2016

Here's a fucking crazy idea - default comparisons to deep. Why are we all typing .deep over and over again when almost no one needs shallow, reference comparison. I would even go as far as come up with a new method for .to.be.reference() to make your intention clear.

I'm so fucking sick of having to fix my tests because of a missing .deep modifier.

@cjihrig

This comment has been minimized.

Copy link
Contributor

@cjihrig cjihrig commented Apr 30, 2016

I agree with this idea. I think it would be simplest to just add a shallow modifier though.

@hueniverse

This comment has been minimized.

Copy link
Member Author

@hueniverse hueniverse commented Apr 30, 2016

I would like both. shallow and reference(). If we keep deep this will only break the very few places where we want to get the same reference. And by break I mean false positive.

@cjihrig cjihrig added this to the 3.0.0 milestone May 22, 2016
@cjihrig cjihrig self-assigned this May 22, 2016
@tanepiper

This comment has been minimized.

Copy link

@tanepiper tanepiper commented May 31, 2016

While this is an appreciated change because deep behavior should be default, this morning I ended up spending two hours trying to figure out why recent tests broke in our latest update, and then refactoring the code to fix it. This change was not made visible (nothing in the release notes or changelog - and the API docs still reference 2.3.0) and no deprecation warning - just a sudden change. Milestones are not that visible in terms of documentation.

@cjihrig

This comment has been minimized.

Copy link
Contributor

@cjihrig cjihrig commented May 31, 2016

My bad on the API doc. The docs were correct, with the exception of the version at the top. That is fixed now.

@tanepiper

This comment has been minimized.

Copy link

@tanepiper tanepiper commented May 31, 2016

@cjihrig Thanks, appreciated (and all the work you've done on Code so far)

@hueniverse

This comment has been minimized.

Copy link
Member Author

@hueniverse hueniverse commented May 31, 2016

@tanepiper so you moved to a new major version and just assumed nothing broke?!

@tanepiper

This comment has been minimized.

Copy link

@tanepiper tanepiper commented Jun 1, 2016

@hueniverse Never assumed nothing would break, but it was not documented clearly - usually major breaking changes are clearly marked in changelogs, and like I said the API docs had not been updated clearly to reflect the version change at it was still referencing v2.3.x which caused some initial confusion.

I had to find out from twitter that it's here in the Milestones section of Code that it's documented that this would be happening, it's not visible from just the issues tab.

If you look at Git, milestones are not exactly the most discoverable way of locating this information:

github

Maybe editing in the release notes, which is easily accessible, to indicate it would have been good. Even a one liner like https://github.com/hapijs/code/releases/tag/v2.2.1

But as I also mentioned this is a welcome and sensible change, so I do appreciate the work done on it.

@hueniverse

This comment has been minimized.

Copy link
Member Author

@hueniverse hueniverse commented Jun 1, 2016

I am not arguing the change was clearly documented, just that I would expect you to first figure out what changed before actually making the transition...

@enjikaka

This comment has been minimized.

Copy link

@enjikaka enjikaka commented Jun 2, 2016

"I would expect you to first figure out what changed"

That's going to be hard if you don't alert the users via release notes or docs. You should never EVER release without updating that, since it causes huge trouble for developers using your code then.

@nlf

This comment has been minimized.

Copy link
Member

@nlf nlf commented Jun 2, 2016

The existing pattern of documenting breaking changes for any hapi related project has always been the milestones. As Eran said, updating without looking to see what changed is what caused the issue here.

However, I would agree that you not looking was likely caused by the fact that finding the milestones is a pain in the ass. I have a couple of ideas to help make this easier for everyone in the future.

As for your comment, @enjikaka, your hostility is not welcome here. We all work on these projects for free in our spare time after we work another full time job. If something isn't right, I know it's frustrating but suggesting ways to resolve it is 10,000,000 times more helpful than telling a maintainer what they can and cannot do. It's open source. If you don't like something, fix it.

@devinivy

This comment has been minimized.

Copy link
Member

@devinivy devinivy commented Jun 2, 2016

This is free software open for improvement by everyone.

In this case the docs were updated, but a simple mistake was made in updating the doc's heading to reflect the new version. The hapijs organization reliably uses milestones to track changes, and in the future if there's any question as to what has changed with a version bump, you can at the very least check there. If you find that a mistake was made, usually a friendly and constructive issue or a PR will do the trick to fix it.

@enjikaka

This comment has been minimized.

Copy link

@enjikaka enjikaka commented Jun 2, 2016

@nlf Not trying to be hostile. If you've ever use other peoples code enough I'm sure you've encountered the frustation when developers do not keep their documentation up to date so things just don't work out for others, and you have to spend a week trying to figure out why. What seems to be a small thing to miss out on can actually have a really big and bad effect for many people. This is the worst kind of bad DX.
I see that it was documented, but not easily accessible. That's important too. I'm not a user of this, but for all that do use it; please keep this in mind next time. Good luck in the future.

@Marsup Marsup added the feature label Sep 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.