search

kschiess / parslet

A small PEG based parser library. See the Hacking page in the Wiki as well.
kschiess.github.com/parslet
MIT License
805 stars 95 forks source link

Spec fixes for Ruby 2.5, 2.6 #199

Closed jethrodaniel closed 4 years ago

jethrodaniel commented 5 years ago

Ran this on 2.6.1, had 2 specs fail due to the Ruby version:

  1. mathn is deprecated since 2.5 - this fixes https://github.com/kschiess/parslet/pull/192
  2. Integer() calls to_i if to_int fails, as of 2.6 - see https://github.com/ruby/ruby/blob/ruby_2_6/spec/ruby/core/kernel/Integer_spec.rb#L22-L36
deivid-rodriguez commented 4 years ago

Hi @kschiess! This PR fixes CI on new rubies, so it sounds like a good change to include? :)

kschiess commented 4 years ago

Hi,

I will fix the two test failures, however: The change re #to_int and #to_i does make the test work, but it does not address the underlying problem. Which is: Slices are supposed to behave like Strings. That means: 

Integer('abc') # => ArgumentError
Integer('123') # => 123
'abc'.to_i     # => 0

I am thinking of the case where you'd parse some kind of outside world argument; you'd want to make sure that something that needs to be an Integer really is. Calling Integer() on that slice seems natural; but that has become (Ruby 2.6) a unicorn method that cannot be influenced and that behaves rather strangely for objects that have both to_int and to_i. In essence, I cannot now override any of these to obtain the desired behaviour anymore in Ruby 2.6.

That's why I decided to release parslet 2.0 soon(ish) and essentially remove this functionality. To make sure you've got something that looks like an Integer, you'll need to Integer(slice.to_s). This breaks the abstraction and I am really sad as to what Ruby has become; when will "1" == 1 as in other languages? But I see no other way of having a version of parslet that behaves the same, no matter what Ruby version it is used with.

Can you amend the PR to change the spec for Integer conversion to 

lambda { Integer(slice.to_s) }.should raise_error(ArgumentError, /invalid value/)

? I'll accept the PR just after that, amending HISTORY to announce the upcoming release.

Thank you for your work and for calling my attention to this.

kaspar