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

Extract Trash behavior into an extension #297

Merged
merged 2 commits into from
Apr 30, 2015

Conversation

michaelherold
Copy link
Member

Issue #188 brought up the fact that a Trash is really just a Dash with
some extended behavior. Why don't we make that a little more explicit
and define its behavior as an extension?

Follow Up

If this is merged, I move that we deprecate Trash for removal in 4.0.
I think reducing the number of classes will help users better understand
what behavior they are including in their subclasses.

In the long run, a 4.0 release would look really good with all of the behavior
factored out into extension methods. As we continue down that path, we can
slowly deprecate all of the *ash classes with the 3.x releases.

What do the other maintainers think of this suggestion?

Fixes #188

Issue hashie#188 brought up the fact that a Trash is really just a Dash with
some extended behavior. Why don't we make that a little more explicit
and define its behavior as an extension?

*Follow Up*

If this is merged, I move that we deprecate Trash for removal in 4.0.
I think reducing the number of classes will help users better understand
what behavior they are including in their subclasses.

In the long run, a 4.0 release would look really good with all of the behavior
factored out into extension methods. As we continue down that path, we can
slowly deprecate all of the `*ash` classes with the 3.x releases.

What do the other maintainers think of this suggestion?

Fixes hashie#188
@dblock
Copy link
Member

dblock commented Apr 27, 2015

This is great, merging.

I wonder whether deprecating *ash classes will just make it harder on users, after-all they are "typical".

@dblock
Copy link
Member

dblock commented Apr 27, 2015

Before I merge ... this needs documentation for the new extension in README.

@michaelherold
Copy link
Member Author

Whoops! Thanks for the reminder.

@michaelherold
Copy link
Member Author

There we go, I added the docs to the readme.

Regarding deprecating the *ash classes:

My reasoning behind that suggestion is that the availability of the classes can lead to them being used in ways that aren't actually their intended use. Since I've been helping with Hashie I have heard the mantra that "*ash is [first and foremost] a Hash". I agree with this sentiment; we are going out of our way to ensure that the *ash classes are Hashes and no Hash functionality is removed.

However, because Hashie has these wonderful classes that automagically allow them to wrap a JSON response, often Hashie -- in particular Mash -- ends up being used in cases where it's not actually appropriate. The controversial blog post by Richard Schneeman -- along with the arguments on Reddit and in the OmniAuth repo -- shows an example of this. Another example is elasticsearch-rails, where Hashie is used to wrap all of the responses from Elasticsearch which causes issues in our application and caused us to want to either make Hashie optional or remove it as a dependency.

I feel that making people choose which extensions they want to use instead of giving the larger classes outright will help make them think more about what they are doing and help lead to using the right tool for the job.

That being said, I know that a lot of our users are using it in this way and have argued in the past that we should cater the ways Hashie is being used now. I'm of split minds on the issue, but I thought that a discussion around this extraction might be a good step for Hashie 4.

I believe that @gregory is in favor of extracting all behavior into mixins, based on comments in that discussion I linked earlier. I can't really speak for anyone else, but others might want to weigh in. I'm interested in your opinion, @dblock, since you raise the good point that it might just make things more difficult for users.

Is further feature extraction like this something we want to pursue? What about deprecating the *ash classes?

Also, should I move this discussion to a new issue?

@dblock
Copy link
Member

dblock commented Apr 30, 2015

I think you should open an issue about deprecating *ash classes, and we can talk it over there (copy-paste ^^^).

dblock added a commit that referenced this pull request Apr 30, 2015
Extract Trash behavior into an extension
@dblock dblock merged commit 1230344 into hashie:master Apr 30, 2015
@gregory
Copy link
Contributor

gregory commented Apr 30, 2015

This is awesome @michaelherold !
And yes, i'm still convinced that it should be modules to include instead of a bundle of obscure and untraceable features wrapped into a class.

jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Jun 9, 2015
## 3.4.2 (6/2/2015)

* [#292](hashie/hashie#292): Removed `Mash#id` and `Mash#type` - [@jrochkind](https://github.com/jrochkind).
* [#297](hashie/hashie#297): Extracted `Trash`'s behavior into a new `Dash::PropertyTranslation` extension - [@michaelherold](https://github.com/michaelherold).

## 3.4.1

* [#269](hashie/hashie#272): Added Hashie::Extensions::DeepLocate - [@msievers](https://github.com/msievers).
* [#270](hashie/hashie#277): Fixed ArgumentError raised when using IndifferentAccess and HashWithIndifferentAccess - [@gardenofwine](https://github.com/gardenofwine).
* [#281](hashie/hashie#281): Added #reverse_merge to Mash to override ActiveSupport's version - [@mgold](https://github.com/mgold).
* [#282](hashie/hashie#282): Fixed coercions in a subclass accumulating in the superclass - [@maxlinc](https://github.com/maxlinc), [@martinstreicher](https://github.com/martinstreicher).
@michaelherold michaelherold deleted the property-translation branch November 28, 2016 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

why does trash exists?
3 participants