Skip to content
New issue

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

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

Already on GitHub? # to your account

Add a validation code to check if json_column_name column is string type #9

Merged
merged 1 commit into from
Mar 4, 2016

Conversation

muga
Copy link
Contributor

@muga muga commented Mar 4, 2016

This fix is to add a validation code for checking if the type of a column specified as json_column_name option is string or not. setExpandedJsonColumns method requires that the json column is string type. It's nice to have such type validation.

To add an unit test for this validation, I removed final modifier from schema field in TestExpandJsonFilterPlugin class. But this change might not be bad for us.

@civitaspo
Copy link
Member

ship it!

civitaspo added a commit that referenced this pull request Mar 4, 2016
Add a validation code to check if json_column_name column is string type
@civitaspo civitaspo merged commit 35d10b4 into embulk:master Mar 4, 2016
@civitaspo
Copy link
Member

this change might not be bad for us

@muga hmm, could you comment on the code the reason the change is needed?
(excuse, I merged already this p-r.)

@muga
Copy link
Contributor Author

muga commented Mar 5, 2016

@civitaspo Ok, I will do that. To be clarified, the reason why I removed final is because my test method requires different schema object from [_c0:string, _c1:string], you know. In the future, I think that new additional test methods also will require different schemas. So, this change is reasonable.

@muga
Copy link
Contributor Author

muga commented Mar 5, 2016

Added comment on the test class. #10

Thank you for merging.

@civitaspo civitaspo mentioned this pull request Mar 16, 2016
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants