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

Prevent Ruby 1.8 error with rescue clause #123

Closed
wants to merge 1 commit into from

Conversation

ggilder
Copy link

@ggilder ggilder commented Jan 10, 2014

In Ruby 1.8, the final line of the math_error_classes method would
return nil if Math::DomainError was not defined. That would cause the
rescue in distance_between_sphere to be effectively rescue *nil, which
produces "TypeError: class or module required for rescue clause".

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 6c74180 on ggilder:fix-ruby-18-error into 9446d23 on geokit:master.

@tamird
Copy link

tamird commented Jan 10, 2014

👍 this causes sadness

In Ruby 1.8, the final line of the math_error_classes method would
return nil if Math::DomainError was not defined. That would cause the
rescue in distance_between_sphere to be effectively `rescue *nil`, which
produces "TypeError: class or module required for rescue clause".
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling ac0f304 on ggilder:fix-ruby-18-error into 9446d23 on geokit:master.

@ggilder
Copy link
Author

ggilder commented Jan 11, 2014

Travis build is failing to bundle on 1.8.7. I'm looking into it though I don't see how it can be related to this change...

@ggilder
Copy link
Author

ggilder commented Jan 11, 2014

Actually, really confused how bundle can possibly be failing since the line in lib/ruby/site_ruby/1.8/rubygems/specification.rb calls compact before sort. How is a nil even getting through??

@ggilder
Copy link
Author

ggilder commented Jan 11, 2014

It looks like Travis has been erroring on all the recent 1.8.7 builds (https://travis-ci.org/geokit/geokit/builds) so I think it's unrelated. It builds fine for me locally.

@mnoack
Copy link
Member

mnoack commented Jan 12, 2014

Thank you @ggilder The ruby 1.8 errors are fixed in the latest rubygems release so that's fine.

2 things:

  1. Could this be fixed by simply adding the line "error_classes"
  2. More importantly what are some inputs (maybe you can help too @tamird) you used to get the error. I'm trying to write a test but it might take a while for me to get an error occurring, then I can write a test and have this code tested.

@ggilder
Copy link
Author

ggilder commented Jan 12, 2014

Sure, adding error_classes at the end of the method would also work, tap just seems more idiomatic for Ruby IMO. As far as reproducing inputs for the error, I have seen the error in a production system but unfortunately our logging isn't showing me the exact lat/lng inputs triggering the bug. I'll try to track that down.

@mnoack
Copy link
Member

mnoack commented Jan 12, 2014

@ggilder @tamird I've addressed the issues in 9c63ba8 Thanks for alerting me to the issue so it could be fixed.

@mnoack mnoack closed this Jan 12, 2014
@tamird
Copy link

tamird commented Jan 12, 2014

@mnoack would you mind cutting a new release with this fix?

@mnoack
Copy link
Member

mnoack commented Jan 12, 2014

No problem. Done 1.8.4 released

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.

4 participants