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

Remove need to initialize child class #7

Closed
wants to merge 2 commits into from
Closed

Remove need to initialize child class #7

wants to merge 2 commits into from

Conversation

pythonandchips
Copy link

Hi

I made a change on how the @eventable_mutex is accessed.

Added a private method that wraps the access to the instance variable that will initialise it if null. This will remove the need for the user to add an initialize method with super which just cleans up the code a bit.

Also added the rspec rake tasks.

Cheers
Colin

…ing the module. wrapped it in private method that will auto initialize it null
@@ -6,12 +6,6 @@ class EventedClass
event :stuff_happens
event :other_stuff_happens

def initialize
Copy link
Author

Choose a reason for hiding this comment

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

This is no longer need and nicely cleans up the class a bit

@mikbe
Copy link
Owner

mikbe commented Jun 29, 2011

That's very similar to how I was doing it originally but unfortunately that is not threadsafe. If two threads call the method that instantiates the mutex at the exact same time they can end up with a different mutex.

See this gist to see how that might happen: https://gist.github.com/1031308

I do greatly appreciate the effort though. Thanks!

@mikbe mikbe closed this Jun 29, 2011
RSpec::Core::RakeTask.new do |t|
t.pattern = "./spec/**/*_spec.rb" # don't need this, it's default.
end

Copy link
Owner

Choose a reason for hiding this comment

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

Good idea, I should be running my specs in the rake file when building the gem but I do continuous testing so didn't think to add them.

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.

2 participants