Skip to content
This repository was archived by the owner on Jul 27, 2025. It is now read-only.

Conversation

dmfilipenko
Copy link

@dmfilipenko dmfilipenko commented Feb 26, 2025

@dmfilipenko dmfilipenko changed the title Dashboard design fixes (#1898) Fix conversion for European format Feb 26, 2025
@henrikbjorn
Copy link

Wouldn't it be easier and less error prone to take the string and make it in to characters, then split on the separator and ignore the delimiter.

Something like this

amount = "-233.002,23"

separator = ","
delimiter = "."

whole, cents = amount.chars.split(separator)
whole.reject! { it == delimiter }

p Float("#{whole.join}.#{cents.join}")

@dmfilipenko
Copy link
Author

@henrikbjorn might be. I just follow existing convention

@henrikbjorn
Copy link

henrikbjorn commented Feb 26, 2025

Maybe you can try

def sanitize_number(value, delimiter: ',', separator: '.')
  return "" if value.nil?

  mains, cents = value.to_s.split(separator)
  mains.gsub!(/[^0-9\#{Regex.escape(delimiter)}\-]/, '')

  "#{mains}.#{cents}"
end

p sanitize_number("123,456.78")
p sanitize_number("-123,456.78")
p sanitize_number("12 456.78")
p sanitize_number("12 456,78", separator: ',', delimiter: '.')

Copy link
Collaborator

@zachgoll zachgoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix!

I think I agree with @henrikbjorn on this one that this is more of a parsing issue than anything.

To merge this, I think we need to:

  • Remove the extra format
  • Update parsing to handle both 1.234,56 and 1234,56 properly
  • Remove the tests here and update the "Parses European number format correctly" test to have an additional CSV row (for total of 2 rows) with each of these formats.

@zachgoll
Copy link
Collaborator

Fix was actually a bit simpler than I had thought—the parsing logic is fine, but the form was not properly applying the configuration. Superseded by #1923

@zachgoll zachgoll closed this Feb 28, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants