Fix: Allow smartSplit() to handle String of chars not just a single char#1761
Fix: Allow smartSplit() to handle String of chars not just a single char#1761wetneb merged 3 commits intoOpenRefine:masterfrom omkarnix:master
Conversation
|
Hi @omkarnix, thanks a lot for this pull request! As discussed in the issue it would be great to have a few tests for this (as you have noticed the test coverage is really poor at the moment so we are trying to improve that for all new changes). Let me know if you need any help. |
| } else { | ||
| return function.call(bindings, args); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fantastic, thanks a lot! As a small note, I think invoke should not need to take the function name as parameter, as it is always FUNCTION_NAME in this class (and the method is private). But that is a cosmetic concern.
There was a problem hiding this comment.
Yeah. Actually I thought we should move the invoke method to some test util class. Also could you please help me understand what is RefineTest and should we extend our control function test classes with RefineTest?
There was a problem hiding this comment.
@omkarnix moving to a test util class would make a lot of sense too.
The RefineTest class is used to create a test workspace and initialize various other things that are required when dealing with projects. For function tests it is not needed to extend it, as function registration is done statically as you have rightly noted.
There was a problem hiding this comment.
Ok. Thanks. I would certainly like to refactor it and move the invoke method to some util class.
I noticed that travis CI build failed due to assertion failure in serializeProcessManager whereas appveyor build succeeded. Could it be a glitch while building on travis-ci? Can we trigger the build manually?
There was a problem hiding this comment.
Fantastic, thanks a lot! As a small note, I think
invokeshould not need to take the function name as parameter, as it is alwaysFUNCTION_NAMEin this class (and the method is private). But that is a cosmetic concern.
@wetneb I agree. It was really not required to create constant for the local static value. So made the change suggested by yo .Also added one more misc change. Can you review it?
There was a problem hiding this comment.
yes don't worry about unrelated test failures (although this one is curious). I can restart the build when it happens.
…independent by replacing BeforeSuite with BeforeTest
|
@wetneb Thank you very much for your support and encouragement on contribution. The community is very supportive. I would certainly try to contribute as much as possible! |
2 participants
This commit adds the support of String separator in function smartSplit. Previously only char separator is supported. Now smartSplit can support String as well as single char as separator. Changed to existing implementation of overloaded constructor of CSVParser which takes string separator.