-
Notifications
You must be signed in to change notification settings - Fork 8
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
convert some string keywords to bool #13
convert some string keywords to bool #13
Conversation
e3529e7
to
bc71e75
Compare
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.
Sorry, I forgot to push submit review
button.
I think it is better to add some strings like the below for compatibility CSV parser plugin behavior.
How do you think?
private static final List<String> BOOL_TRUE_STRINGS = ImmutableList.of("true", "1", "yes","y","t","on");
@@ -25,6 +29,8 @@ | |||
implements ColumnVisitor | |||
{ | |||
private static final JsonParser JSON_PARSER = new JsonParser(); | |||
private static final List<String> BOOL_TRUE_STRINGS = ImmutableList.of("true", "1", "yes"); |
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.
The CSV parser treat the following value as true
.
So I think it is better to add a some keywords (ex On
) for true value.
How do you think?
ImmutableSet.of(
"true", "True", "TRUE",
"yes", "Yes", "YES",
"t", "T", "y", "Y",
"on", "On", "ON",
"1");
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.
-
Keywords will be compared with case-ignored, so we don't need all case variations
https://github.com/hiroyuki-sato/embulk-parser-jsonpath/pull/13/files#diff-2d0fb975204a675b52cc8ea532734971R90 -
Adding more words (e.g. 'on', 'y') is good idea. I'll handle it.
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.
BTW: Is it better to follow CSVParserPlugin to convert string keywords to bool? What do you think about this?
@hiroyuki-sato Thanks for your review. As we discussed on Twitter, I added more words to convert to bool. |
Thank you! |
Thank you! |
This p-r adds new feature that converts some string keywords to boolean value.
embulk-json-path has similar feature, added at takumakanari/embulk-parser-json#1.