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

Add setting for Registration::Object default prefix #105

Merged
merged 7 commits into from
Aug 2, 2015
Merged

Add setting for Registration::Object default prefix #105

merged 7 commits into from
Aug 2, 2015

Conversation

nepalez
Copy link
Contributor

@nepalez nepalez commented Mar 2, 2015

Hi, Kris!

I've tried to add configuration setting to Wisper::Configuration to make
it possible changing default prefix of Registration::Object.

To prevent the Object class from messing by the default setter, I've extracted
the Prefix as value object. Then I've added a method to the Configuration,
because it seems the better solution, than accessing the internal class directly.

I've split changes to 3 commits for every single step of my refactoring.

I didn't discussed it in advance, 'cause it turned out to be fast and simple.
If you find it appropriate, please accept the commits.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 98.73% when pulling 0fb3a68 on nepalez:master into 00fe31e on krisleech:master.

@krisleech
Copy link
Owner

In principle I'm happy with this, thanks 😄 I'll take a closer look as soon as possible. There is one failure on ruby-head. Would you take a look please?

The specs became non-mutating the Prefix.default value.
@nepalez
Copy link
Contributor Author

nepalez commented Mar 3, 2015

D'oh! This was due to inaccurate insulation of my specs from each other.
Corrected.

Instances of Wisper::ValueObject::Events quack like arrays
in respond to #include?(event).
@nepalez
Copy link
Contributor Author

nepalez commented Mar 3, 2015

And one more little thing.

I've noticed the list of events can be set as "strict" String, Symbol, Array,
or Regex only.

For example, when I use not a String, but its subling, I raise ArgumentError:

Event = Class.new(String)
event = Event.new "foo"

event.is_a? String # => true
event # => "foo"

BlockRegistration.broadcast event, #...
# => ArgumentError "Event not supported for `on` argument"

This is because Registration#should_broadcast? checks the class name directly,
via class.name, not is_a? or is_kind_of?.

And once again, before solving the problem I've isolated Events as a
value object quacking like an array in responce to #include?.

I didn't figure out yet how to made my change
(use is_a? instead of class.name) to be well-written,
so I postponed it for a while.

But IMHO the extraction makes Wisper::Registration more readable and testable,
so is could make sense by itself.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.41%) to 99.07% when pulling 1c267c1 on nepalez:master into 00fe31e on krisleech:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.17%) to 98.83% when pulling 1c267c1 on nepalez:master into 00fe31e on krisleech:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.34%) to 100.0% when pulling 8153e99 on nepalez:master into 00fe31e on krisleech:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+1.34%) to 100.0% when pulling 8153e99 on nepalez:master into 00fe31e on krisleech:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.34%) to 100.0% when pulling 8153e99 on nepalez:master into 00fe31e on krisleech:master.

@krisleech
Copy link
Owner

I've not forgot about this.

krisleech added a commit that referenced this pull request Aug 2, 2015
Add setting for Registration::Object default prefix
@krisleech krisleech merged commit bd10b0f into krisleech:master Aug 2, 2015
@krisleech
Copy link
Owner

Thanks and sorry for the delay!

@krisleech
Copy link
Owner

Does this need something adding to the README? Thanks.

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.

3 participants