Skip to content

remove the need for space between number and units#16

Closed
twalpole wants to merge 1 commit intoolbrich:masterfrom
twalpole:remove_space_requirement
Closed

remove the need for space between number and units#16
twalpole wants to merge 1 commit intoolbrich:masterfrom
twalpole:remove_space_requirement

Conversation

@twalpole
Copy link
Contributor

Remove the need for space between number and units, otherwise things like Unit.new("1.4g") throw a stack level too deep error. The requirement seems overly restrictive for no real gain that I can determine

@olbrich
Copy link
Owner

olbrich commented Jun 26, 2011

I've been testing this out and so far it seems pretty solid. There are a couple of cases where the units don't parse properly (like when the unit is something like "1 1/s").

@twalpole
Copy link
Contributor Author

I get the same results for Unit.new("1 1/s") with and without this change - scalar=1 units="1/s" - what result would you expect from it? Also it passed all the specs, so if there are things that its not parsing properly I will add specs for those

@olbrich
Copy link
Owner

olbrich commented Jun 26, 2011

My point is that Unit.new("11/s") doesn't work well.

@twalpole
Copy link
Contributor Author

I get the same results for Unit.new("11/s") on both the master branch and the branch including my change. scalar=11 unit=1/s - this is using ruby 1.8.7-p174 - Are you seeing different results, or should the results be different than what I'm seeing? I'm more than willing to fix any issues if you will just point out specifically what issue you're finding - thanks

@olbrich
Copy link
Owner

olbrich commented Jun 26, 2011

I would expect that the scalar on that should be 1, not 11. Although this gets ambiguous as "11/m" is a valid way to express "11 1/m".

specify { Unit("11/m").should == Unit('1 1/m')}

@twalpole
Copy link
Contributor Author

Okay, but then this fails on master without the change I propsed too,
and I would say that 11/m should be interpreted as scalar 11 unit 1/m
just as 11m/s is scalar 11 units m/s not scalar 1 units 1m/s

@gentooboontoo
Copy link
Contributor

I don't know if it is related but I also have a stack level too deep error in the following cases:

Unit test (puts is needed to trigger the failure)

def test_puts_then_create_float_failure
  puts 'does not fail if commented out'
  Unit.new('1.5m') # <= fails only if scalar is not an integer
  #=> SystemStackError: stack level too deep
end

In irb console, failure occurs with:
>> U('1.5m')
but not with
>> U('1.5 m')

@twalpole
Copy link
Contributor Author

Yep - thats the issue this change fixes

On Fri, Jul 29, 2011 at 9:35 AM, gentooboontoo
reply@reply.github.com
wrote:

I don't know if it is related but I also have a stack level too deep error in the following cases:

Unit test (puts is needed to trigger the failure)

   def test_puts_then_create_float_failure
     puts 'does not fail if commented out'
     Unit.new('1.5m') # <= fails only if scalar is not an integer
     #=> SystemStackError: stack level too deep
   end

In irb console, failure occurs with:
   >>   U('1.5m')
but not with
   >> U('1.5 m')

Reply to this email directly or view it on GitHub:
#16 (comment)

@gentooboontoo
Copy link
Contributor

Thanks. Your change seems correct. '1.5m' is perfectly valid but it fails with IRB and under some conditions I can't reproduce in a test without a 'puts'.

hynkle pushed a commit to hynkle/ruby-units that referenced this pull request Sep 7, 2011
@Sija
Copy link

Sija commented Mar 11, 2013

Hi, bug is still here:

Unit('1.5kg') => SystemStackError: stack level too deep

Any chances on pulling this up?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants