Skip to content

"1.".to_yaml #109

Closed
glebm opened this Issue Jan 2, 2013 · 9 comments

2 participants

@glebm
glebm commented Jan 2, 2013

According to YAML spec "1." is a float (http://yaml.org/type/float.html)
However, in ruby Float("1.") fails

Therefore "1.".to_yaml fails at https://github.com/tenderlove/psych/blob/master/lib/psych/scalar_scanner.rb#L99

Related (jruby): jruby/jruby#474

@glebm
glebm commented Jan 9, 2013

Is there anything I need to do to get this merged?

@tenderlove
Owner

Sorry to take so long. I'll check it out and merge it in. :-)

@glebm
glebm commented Jan 9, 2013

I know you've been busy with more pressing issues! 👍 Thanks!

@tenderlove
Owner

It definitely seems that "1." is a valid float in yaml. With your patch applied, Psych will return a string rather than the expected float:

irb(main):001:0> require 'psych'
=> true
irb(main):002:0> Psych.load '--- 1.'
=> "1."

We should probably make this return the float as the YAML spec says. What do you think of this patch?

diff --git a/lib/psych/scalar_scanner.rb b/lib/psych/scalar_scanner.rb
index d0beee3..5759459 100644
--- a/lib/psych/scalar_scanner.rb
+++ b/lib/psych/scalar_scanner.rb
@@ -96,7 +96,7 @@ module Psych
           @string_cache[string] = true
           string
         else
-          Float(string.gsub(/[,_]/, ''))
+          Float(string.gsub(/[,_]|\.$/, ''))
         end
       else
         int = parse_int string.gsub(/[,_]/, '')
diff --git a/test/psych/test_numeric.rb b/test/psych/test_numeric.rb
index 200a9f0..0858e79 100644
--- a/test/psych/test_numeric.rb
+++ b/test/psych/test_numeric.rb
@@ -16,6 +16,10 @@ module Psych
       $DEBUG = @old_debug
     end

+    def test_load_float_with_dot
+      assert_equal 1.0, Psych.load('--- 1.')
+    end
+
     def test_non_float_with_0
       str = Psych.load('--- 090')
       assert_equal '090', str

With this patch applied:

irb(main):001:0> require 'psych'
=> true
irb(main):002:0> Psych.load '--- 1.'
=> 1.0
@glebm
glebm commented Jan 9, 2013

Yes, but with this patch it doesn't round-trip properly.

We have a normal String represented in the model as part of the address, and we use YAML to pass it to our geolocation server, so we don't really want it to get converted to a float ("1." -> 1.0) when it was a string to begin with.

Maybe the dumping bit can be modified to always dump as string when given a string? Not sure if this is possible in this case

(now it just raises an exception, so either one is better than the current code.)

@glebm
glebm commented Jan 9, 2013

Basically, I want this to work: YAML::parse(YAML::dump("1.")) #=> "1."
But you are right that we should be conforming to YAML spec too

I guess we need to dump "1." differently?

@tenderlove
Owner

No, with my patch, it will roundtrip properly:

irb(main):004:0> x = '1.'
=> "1."
irb(main):005:0> x == Psych.load(Psych.dump(x))
=> true
irb(main):006:0> Psych.dump x
=> "--- '1.'\n"
irb(main):007:0> Psych.load '--- 1.'
=> 1.0
irb(main):008:0>
@glebm
glebm commented Jan 9, 2013

I see, should've tested it first :)
In that case 👍, 🍻

@tenderlove
Owner

Awesome! I'll apply the patch then. Thanks for bringing this issue up! :-D

@tenderlove tenderlove added a commit that closed this issue Jan 9, 2013
@tenderlove * ext/psych/lib/psych/scalar_scanner.rb: strip trailing dots from
  floats so that Float() will not raise an exception.

* test/psych/test_numeric.rb: test to ensure "1." can be loaded

* test/psych/test_string.rb: make sure "1." can round trip

fixes #109
cf82e48
@tenderlove tenderlove closed this in cf82e48 Jan 9, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.