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

Support import gzip json file #1

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

Conversation

ndinh215
Copy link
Owner

@ndinh215 ndinh215 commented Jan 7, 2016

No description provided.

)
->addOption(
'gzip',
null,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use zshortcut, it should be available. When we were discussing about shortcuts with multiple letters, the problem was that one of the letter was reserved, that's why there was a mistake with multiple. Since z is free it could be added. btw z in most of the unix commands means compressing.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Thanks @saimaz .

@saimaz
Copy link

saimaz commented Jan 7, 2016

In terms of implementation I have some concerns about isGzip variable passed through all classes. not sure yet is this a best way or there is another better one. @mvar @murnieza ?

@ndinh215
Copy link
Owner Author

ndinh215 commented Jan 7, 2016

Firstly, I did, I named it 'dataType', it could be XML, GZIP or any format. Then I thought 'isGzip' could reflect directly our expectation. For further extension, I think 'dataType' is OK.

@mvar
Copy link

mvar commented Jan 7, 2016

@saimaz me too. What if in the future we need to add more options? Maybe we should pass $options array here?

@ndinh215 have you noticed, that you created this PR in your fork?

@ndinh215
Copy link
Owner Author

ndinh215 commented Jan 7, 2016

@mvar, this branch is based on the newest master in my repo. I still wait for the reviewing the issue ongr-io#535, then I will merge and then create new PR for this issue.

@murnieza
Copy link

murnieza commented Jan 7, 2016

I also think that all options (should we call it context?) should be passed around in one array but not separately.

@ndinh215
Copy link
Owner Author

ndinh215 commented Jan 7, 2016

So now, there are many names: 'options', 'context', 'params' ... for options array.
So which name do you prefer? @saimaz

@saimaz
Copy link

saimaz commented Jan 7, 2016

Maybe function should look loke this:

protected function getReader($manager, $filename, $convertDocuments, $options)

@ndinh215
Copy link
Owner Author

ndinh215 commented Jan 7, 2016

OK, thks @saimaz

@ndinh215
Copy link
Owner Author

ndinh215 commented Jan 8, 2016

I complemented this case with the param 'options'. Could you please review again these changes before I merge and create a new PR to ongr-io repo?@saimaz
Thanks.

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