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

Modified the GuessDelimiter function. #593

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Modified the GuessDelimiter function. #593

wants to merge 11 commits into from

Conversation

ErikSimonsen
Copy link

Changed the GuessDelimiter function, so that a new delimiter is chosen as best if its delta value is higher than the current chosen.
Added a testcase for decimal values with comma as decimal separator and pipes as delimiter.
(Which should mimic the following test data:
a|3,4|b
c|3,4|d
)

But i couldnt ran the tests because i dont have a node interpreter installed, so i am not sure if the other tests break, altough the test data mentioned above now produces the correct result).

Changed the GuessDelimiter function, so that a new delimiter is chosen as best if its delta value is higher than the current highest.
Added a testcase for decimal values with comma as decimal separator and pipes as delimiter.

But i couldnt ran the tests as i dont have a node interpreter installed, so i am not sure if the other tests dont break now.
@pokoli
Copy link
Collaborator

pokoli commented Nov 2, 2018

Hi, thanks for the PR I see you've added a new test-cases.js file, you should modify the tests-cases.js file in the test folder.

Added testcase with pipes and decimal values with commas as decimal separator.
@pokoli
Copy link
Collaborator

pokoli commented Nov 6, 2018

We use tabs for formating and you just added some code with spaces. This breaks the linter , could you please fix it?

@pokoli
Copy link
Collaborator

pokoli commented Nov 6, 2018

I will prefer to include only in this pull request the changes related to the GuessDelimiter function and move all the other code styling changes in a separate Pull Request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants