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

fix rspec 3.3 #32

Merged
merged 3 commits into from
Jul 9, 2015
Merged

fix rspec 3.3 #32

merged 3 commits into from
Jul 9, 2015

Conversation

michaelglass
Copy link
Contributor

split this off from #27

def clear_memoized
# __memoized is private method and is defined in rspec 3.3
__memoized.instance_variable_get(:@memoized).clear
rescue NameError
Copy link
Contributor Author

Choose a reason for hiding this comment

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

don't use rescue for control flow

@michaelglass michaelglass changed the title clean let definitions fix rspec 3.3 Jul 9, 2015
@michaelglass
Copy link
Contributor Author

@eitoball can you take a look at my mild changes?

@michaelglass michaelglass mentioned this pull request Jul 9, 2015
@@ -8,13 +8,22 @@ def clear_exception
end
end

require 'pry'
Copy link
Contributor

Choose a reason for hiding this comment

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

Should remove this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is why we code review thanks.

@eitoball
Copy link
Contributor

eitoball commented Jul 9, 2015

Almost LGTM, see #32 (comment)

I intentionally didn't use __init_memoize, but I now think that using it is much better. Thanks!

michaelglass added a commit that referenced this pull request Jul 9, 2015
@michaelglass michaelglass merged commit 343bf99 into master Jul 9, 2015
@michaelglass michaelglass deleted the rspec-3.3 branch July 9, 2015 23:19
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