Skip to content
This repository has been archived by the owner on Nov 30, 2024. It is now read-only.

Define Hash subclasses for @inclusions and @exclusions #1044

Merged
merged 1 commit into from
Aug 15, 2013

Conversation

dchelimsky
Copy link
Contributor

Building on changes from #1038, this clarifies the differences
between the @Inclusions and @exclusions hashes in FilterManager
by introducing Hash subclasses with behavior appropriate to each.

STANDALONE_FILTERS = [:locations, :line_numbers, :full_description]

module Describable
class InclusionFilterHash < Hash
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm generally not a fan of subclassing core types. @steveklabnik touched on this recently:

http://words.steveklabnik.com/beware-subclassing-ruby-core-classes

Even if we don't run into any of the specific problems mentioned there, it has enough gotchas that I reach for it only after deciding against other options (particularly composition) for whatever reasons.

Is it really needed here? Can we use composition instead?

Building on changes from #1038, this clarifies the differences
between the @Inclusions and @exclusions hashes in FilterManager
by introducing Hash subclasses with behavior appropriate to each.
@kwstannard
Copy link

I just saw this the other day. http://words.steveklabnik.com/beware-subclassing-ruby-core-classes

@dchelimsky
Copy link
Contributor Author

@kwstannard - the code before your changes and mine was extending the inclusions and exclusions hashes with modules, which suffers the same problems. @myronmarston and I had a brief convo about this but I think it's no longer accessible because I force pushed to this PR's branch. Basically - these changes do what they're supposed to - clarify the behavior of two different hashes; I will also look into a wider reaching refactoring that hides the hashes entirely in a separate PR. That all make sense?

@kwstannard
Copy link

Yeah, that makes sense. Thanks.

@myronmarston
Copy link
Member

@kwstannard -- FWIW, I don't see this PR as adding new core-type inheritance. We already had inheritance in the form of the module extensions we were doing before. The difference is that extend SomeModule is implicit doesn't-really-look-like-inheritance-but-actually-is whereas < Hash is obvious, explicit inheritance. Using extend also blows away MRI's method cache, which can cause potential perf issues. I hope we'll get away from core-type inheritance eventually here but this is an improvement over what we have. Iterative improvement FTW :).

myronmarston added a commit that referenced this pull request Aug 15, 2013
@myronmarston myronmarston merged commit 0029d5c into master Aug 15, 2013
@myronmarston myronmarston deleted the refactor-filter-manager-hashes branch August 15, 2013 15:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants