Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable Defining A Custom Thousand Separator Format #30

Conversation

ahmedmsvb
Copy link
Contributor

Java Script parseFloat method ignores comma when parsing a string number
For some locales, the amount is written in format 1,234.56
The above will not parse correctly unless ynab-buddy was instructed with the correct separator format

When a date is surrounded by space, the date library fails to parse it
This commit is to trim the date field before parsing date fields.
Java Script parseFloat method ignores comma when parsing a string number
For some locales, the amount is written in format 1,234.56
The above will not parse correctly unless ynab-buddy was instructed with the correct separator format
…rge branch 'nielsmaerten-main' into feature/allow-custom-thousand-separator-for-amounts
@codecov
Copy link

codecov bot commented Dec 21, 2021

Codecov Report

Merging #30 (95b087d) into main (4dd8269) will increase coverage by 0.17%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #30      +/-   ##
==========================================
+ Coverage   94.39%   94.57%   +0.17%     
==========================================
  Files          10       10              
  Lines         857      884      +27     
  Branches       63       66       +3     
==========================================
+ Hits          809      836      +27     
  Misses         46       46              
  Partials        2        2              
Impacted Files Coverage Δ
src/lib/parser.spec.fixtures.ts 100.00% <100.00%> (ø)
src/lib/parser.ts 96.24% <100.00%> (+0.51%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4dd8269...95b087d. Read the comment docs.

@nielsmaerten nielsmaerten self-requested a review December 21, 2021 20:00
@nielsmaerten nielsmaerten self-assigned this Dec 21, 2021
@nielsmaerten nielsmaerten added the bug Something isn't working label Dec 21, 2021
@nielsmaerten
Copy link
Owner

Thanks for spotting this!

I'll take a closer look at this later, but so far I've added another test in branch fix/parse-amounts.
This test tries parsing using a number of different formats, which seems to work nicely with your fix.

For backwards compatibility, we'll have to use the 'old' way of parsing when neither decimal nor thousand separators are defined though.

@ahmedmsvb
Copy link
Contributor Author

ahmedmsvb commented Dec 21, 2021

Glad that it worked with all the new tests!
Let me know if I can help in finalizing this PR.

And, of course, thanks for this great and useful repo ;)

Copy link
Owner

@nielsmaerten nielsmaerten left a comment

Choose a reason for hiding this comment

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

Hi @ahmedmsvb
I made a few minor changes:

  • additional test to assert correct parsing of a bunch of different formats
  • added the separator options to the default config file
  • fallback to the original behavior if no separator is defined. this will allow existing users to upgrade without having to change their config file
  • strip out non-number characters (eg "€ 1234.56" => "1234.56")

@ahmedmsvb
Copy link
Contributor Author

Hi @nielsmaerten

Looks more robust now.
I ran the new test locally with negative values as well and it passed (don't think we need to duplicate the test with negative numbers)

The decimals fallback is a good idea as long as it is mentioned in the docs

@nielsmaerten
Copy link
Owner

Updated the docs: it now says leaving out the decimal separator isn't recommended. But that if you do skip it, the tool will try to "auto-detect" the separator. (Went with "auto-detect" bc I don't want the docs to be too technical).

Since the settings for decimal and thousands separators will now appear in the default config file (the one that gets created when ynab-buddy runs for the first time), we can already be pretty sure people will fill in this value.

@nielsmaerten nielsmaerten merged commit 1a509c1 into nielsmaerten:main Dec 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants