-
Notifications
You must be signed in to change notification settings - Fork 2
add json connector #48
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
base: 1.x
Are you sure you want to change the base?
Conversation
|
Hi Anzarwani, awesome to have a PR for a new element! And thanks for making it similar to existing input elements. We haven't put much guidance on PR yet but it's very close from the start, well done! I will start the code review comments but in the meantime, here some points to consider:
Thanks for any answer you can share. |
| * `IMAGE` :octicons-arrow-both-24: `("Image", ".jpg .png .jpeg")` | ||
| * `ZIP` :octicons-arrow-both-24: `("ZIP", ".zip .gz .tar.gz .7z")` | ||
| * `JSON` :octicons-arrow-both-24: `("JSON", ".json")` | ||
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.
extra blank line to be removed
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.
causes flake8 error:
onecode/base/enums.py:126:1: W293 blank line contains whitespace
| from ..input_element import InputElement | ||
|
|
||
|
|
||
| class JSONReader(InputElement): |
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.
Would you be ok renaming the component to JsonTableReader or JsonRowReader or similar?
The expected JSON is a table-like which is a subset of the variety of JSON. I am afraid JSONReader would be misleading in the sense it would tackle any JSON (even non-table-like).
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.
Hi
Sorry I had been busy with production issues at work. Please give me some time, I will walk through the comments and the code, and accordingly update you here.
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.
no worries and no rush :)
whenever it's a good time for you
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.
this file is only necessary if there are emulations tests. I would either drop it, or add emulations test (the former being easier ;-))
|
Some comments related to failing CI:
Thanks a lot for the PR, let me know what you think about all the comments. |
Added JSON connector as an enhancement.
Testing is pending as I am facing some issues with module loading, will fix it soon.