parsing amounts with a trailing minus sign #133

Merged
merged 1 commit into from Dec 20, 2011

Conversation

Projects
None yet
4 participants
Contributor

nubs commented Dec 16, 2011

In accounting, it is not uncommon to see the minus sign after the amount rather than before. For example: "$500.42-" instead of "-$500.42" or "$-500.42". Currently, this raises an error: "ArgumentError: Invalid currency amount (hyphen)" but I don't see why it would not be an allowable input.

Member

dom1nga commented Dec 16, 2011

why not normal for ruby using something like this var = 8-?

Member

dom1nga commented Dec 16, 2011

with this pull request Money.parse('-5.95-') # => #<Money cents:-595 currency:USD>
i think this is not normal behavior.

Owner

semmons99 commented Dec 16, 2011

@nubs Could you please add some tests for this?

@dom1nga what would you expect Money.parse('-5.95-') to do?

The problem I have with #parse in general is that we try and throw too much at it and hope it converts properly. If I was doing this over, there wouldn't be a parse method. I'd expect the user to know their input format and get it into some form of Numeric ahead of time.

Member

dom1nga commented Dec 16, 2011

@semmons99 lambda { Money.parse('-5.95-') }.should raise_error ArgumentError or Money.new(0, 'USD')

Owner

semmons99 commented Dec 16, 2011

@dom1nga I don't really have an opinion one way or another; but could you elaborate more on why you think this should throw an ArgumentError?

Member

dom1nga commented Dec 16, 2011

when you take number as part of string argument, you can expect as a result of non-zero value. better to raise argument error, than to mislead with zero value as a result.

Owner

semmons99 commented Dec 16, 2011

@dom1nga sounds reasonable. I assume you would argue the - in front and behind the number part is ambiguous and could either be a negative or some type of separator from data fields, much like a comma in a standard CSV?

I wonder if we need to raise a new ParsingError for anything Money#parse can't handle. Does such a thing exist?

Member

dom1nga commented Dec 16, 2011

it would be nice.
but in this case we have to override result of something like this Money.parse ('hellothere'), and this can create problems for current users. with accepting this issue we'll have these problems. I am against minus sign after the amount because of the mathematical perception of numbers. -8 is negative 8.
8- or 8 - is mathematical operation, which expects the second argument.

Contributor

nubs commented Dec 19, 2011

Updated pull request with tests for negative amounts in general. Changed code to throw an argument error for "-$5.95-"

Owner

weppos commented Dec 19, 2011

+1 for ParseError. The ArgumentError is quite generic and can't be handled very well at higher levels.

spec/money/parsing_spec.rb
@@ -51,6 +51,16 @@
Money.parse('hellothere').should == empty_price
Money.parse('').should == empty_price
end
+
+ it "should handle negative inputs" do
@weppos

weppos Dec 19, 2011

Owner

Please don't begin examples names with the word 'should'. It's redundant and results in hard to read spec output.

it "handles negative inputs" do

Also, I encourage you to split assertions with different meanings into separate groups.

it "raises ArgumentError when unable to detect polarity" do
  lambda { Money.parse('-$5.95-') }.should raise_error ArgumentError
end
@nubs

nubs Dec 19, 2011

Contributor

Makes sense. Updated

Parse 500.42- as a negative amount.
In accounting, it is not uncommon to see a trailing minus sign used for
negative amounts.  Support this method of negative currencies.
Owner

semmons99 commented Dec 19, 2011

@nubs, awesome the patch is starting to come together. Now if you can just raise a ParsingError we'll be all set.

Owner

semmons99 commented Dec 20, 2011

@nubs nevermind about the ParsingError we'll address that issue in a future release.

semmons99 added a commit that referenced this pull request Dec 20, 2011

Merge pull request #133 from nubs/master
parsing amounts with a trailing minus sign

@semmons99 semmons99 merged commit 1395dbc into RubyMoney:master Dec 20, 2011

Owner

semmons99 commented Dec 20, 2011

Thanks for your patch @nubs. You now have commit rights to the repo.

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