-
-
Notifications
You must be signed in to change notification settings - Fork 34
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
Issue #24: Add splitMapper + customisable options #50
Conversation
…Add splitMapper itemMapper, Add tests+documentaion+samples
Codecov Report
@@ Coverage Diff @@
## master #50 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 21 23 +2
Lines 311 351 +40
Branches 5 5
=========================================
+ Hits 311 351 +40
Continue to review full report at Codecov.
|
@Metastasis @Gontrum want you review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, @Siemienik :-)
I only had two ideas. You can see if they make sense.
README.md
Outdated
@@ -79,10 +81,14 @@ interface Person { | |||
FirstName: string; | |||
SecondName: string; | |||
Age: number; | |||
|
|||
EmployedIn: string; | |||
IsUnemployed:boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one little thing for consistency: maybe you want to add a space here and in the next line.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok see it, done in 971bc90. Good catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Metastasis @Gontrum want you review?
LGTM 👍
[''] | ||
] | ||
|
||
// building a mapper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just an idea: Maybe you could add a function buildMapper()
which returns the wordsInSentencesMapper, so you would not need the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I always prefer self-descriptive function than comments, however it is test case code and I wouldn’t make it more complicated. Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it's just a matter of taste and it always depends. I tend to always extract a method, if there is a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done 855d198
Basic todo:
Additionally done:
.separator(';'): SplitMapper
- set separator.itemMapper(itemMapper): SplitMapper
- set mapper for items,fully resolves: #24