Custom delimiters/separators ignored

[vemv]

The following screenshot should be self-explanatory of the problem:

I'd say that

• MoneyRails configuration options are entirely ignored by this gem; and
• Current implementation is not made to handle user-specified delimiters/separators

What do you think?

Cheers - Victor

[antstorm]

@vemv this gem was born as an internal dependency of money-rails and later on extracted into a separate gem. Also monetize was made to be sort of option-less, but it might be worth specifying these to avoid confusion.

[vemv]

You'll agree that it's nearly impossible to have a correct optionless currency lib...

I guess the current implementation works fine under a few assumptions that cover 90% of the cases. But one may want 100% complete/reliable code when it comes to money!

Would a large-ish PR be welcome?

• Observe MoneyRails settings if available
• Accept and correctly implement delimiter/separator options

I assume the test suite gives enough confidence for refactoring.

Cheers - Victor

[antstorm]

@vemv yeah, makes sense, but:

• monetize should not depend on money-rails, so I would say it's up to money-rails (which does depend on monetize) to pass options down to monetize which it should respect
• default_format should not affect parsing and there should be a separate set of settings for that

So it looks like 2 separate changes:

1. Make Monetise#parse accept settings and have default parsing options
2. Add new set of options to MoneyRails that are passed down to Monetize

Sounds sane?

[vemv]

Hey there! Interesting, thanks for the reply.

Make Monetise#parse accept settings and have default parsing options

I don't particularly agree, as I cannot imagine business cases where delimiter and parse_delimiter would be different. That said, adding these two extra options to one's money.rb initializer is so little work, that this is not worth discussing much about.

One upside of having the delimiter/parse_delimiter distinction is that Monetize would avoid breaking API changes. OTOH there's semver...

Add new set of options to MoneyRails that are passed down to Monetize

How would this work? Imagine I call Monetize.parse('42'). How would MoneyRails have instructed Monetize to use a given parse_delimiter/parse_separator?

My guess is that at init time, some Monetize constant-like value would be mutated by MoneyRails. Is that what you had in mind?

...it doesn't sound too bad, but following the if defined?(MoneyRails) pattern might be cleaner.

Cheers - Victor

[antstorm]

1. I think it's quite common to be able to accept any input (which is how it works right now), but ensure that the output is consistent. The reason I think it might be a good idea to separate these is that you don't want your format (output) options to affect your input. And this is why I'm not sure if coupling these two things together is a good idea.

MoneyRails uses Monetize quite a lot when dealing with user input, but formatting is down to Money, which is a bit of a mess, but that's also why unifying these options might bring undesired side-effects.

2. So usually in the rails world you put your settings into an initializer. So along with your default_format, you would provide default_parse (with a better name, probably :) ) that are passed directly to Monetize. So if you use Monetize directly (without money-rails) you can still use these options, but also you don't need to be aware of internal dependencies (which monetize is) when using only money-rails. So yeah, I was thinking about MoneyRails mutating configuration options of the Monetize.

Also keep in mind that money-rails users are probably using the '$99.99'.to_money approach

[vemv]

Agree with both points, thanks for the explanations!

Turns out that for my use case, finally the issue is quite tolerable (because we handle it smartly) so it's not prioritary for me.

Feel free to close (if I had all the time in the world I'd defo open a PR!)

Cheers - Victor

[antstorm]

Oh, cool, happy that you've resolved this. Can you please share some details on how you were able to handle it? Might be helpful for others.

Overall I do think that there needs to be a way of customising Monetize.parse and you've given a very good example.

[JanBussieck]

Hi, I, too, came up against this issue recently. I was frankly surprised that the specified delimiters have no import in how a value is parsed. I could try my hand at a PR for the behavior described above i.e. adding a parse_delimiter option.

[antstorm]

@JanBussieck sounds great! Keep in mind that this should be locale-dependent (not currency-dependent). Happy to assist further if you have any questions.

[JanBussieck]

Great, thanks for your prompt response and the pointer!