Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

readability refactoring of #filter_applies? #528

Merged
merged 1 commit into from Dec 7, 2011

Conversation

Projects
None yet
2 participants
Contributor

AlexKVal commented Dec 6, 2011

Not sure about methods naming.
If those 'metadata' params (like in: filter_applies?(key, value, metadata=self) or configure_for_example(description, user_metadata) and others) would be Metadata, then code would be like:
metadata.filter_for_locations?(value)
metadata[key].filter_applies?(k, v)
user_metadata.configure_for_example(description)
and so on. Much concise and OOP.

Question:
It would be nice if filtering methods
"any_apply? all_apply? filter_applies?"
and private "relevant_line_numbers" and others
move into dedicated module ? (MetadataFiltering may be)
The Metadata class would be cleaner and the filtering logic would be isolated.

and: filter_applies? - it seems it has to be private, but it's not big deal :)

Owner

dchelimsky commented Dec 7, 2011

I don't like the way filter_applies? ends up here, but I like part of the direction it takes it. I'm going to merge this, but make some more changes so you'll want to rebase against master after I'm done.

@dchelimsky dchelimsky added a commit that referenced this pull request Dec 7, 2011

@dchelimsky dchelimsky Merge pull request #528 from AlexKVal/meta
readability refactoring of #filter_applies?
42f2e1c

@dchelimsky dchelimsky merged commit 42f2e1c into rspec:master Dec 7, 2011

Owner

dchelimsky commented Dec 7, 2011

FYI - I started to do the follow up refactoring but I need to pause for a while. I'll attach the commit to this pull when I get there.

Contributor

AlexKVal commented Dec 7, 2011

ok. so I will switch onto other parts of the RSpec for now.
Thank you !

Owner

dchelimsky commented Dec 7, 2011

@AlexKVal - there's more to do here, but take a look at 742a658 - I don't like the nested cases in filter_applies?, but I think this is cleaner than what was there before, which also had nested cases, but they were more interdependent. WDYT?

Owner

dchelimsky commented Dec 7, 2011

D'oh! Works in ruby-1.9.3, but not 1.9.2 :)

Contributor

AlexKVal commented Dec 7, 2011

Added 1.9.3 environment for testing. 1.8.7 1.9.2 1.9.3 - all tests are passed

Owner

dchelimsky commented Dec 7, 2011

@AlexKVal - yes - I fixed it in 1.9.3 in a7d6acd

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment