-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: csv support for conversiont to json #48
Conversation
@gbowne1 if you need/want anything else added to this pr feel please add it to the list in opening 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.
- checked out this feature locally for testing and review for merge
A visual reading of this feature add code does not seem to have any issues with readability and everything looks good and is well structured.
More testing will be necessary but csv to json or jsonc conversion and json or jsonc to csv conversion with data, but this code produces good results as is even at this WIP stage.
A feature like this is great for JSONMaestro because it will allow the user to convert data then possibly even use JSONMaestro on the output further to get their data in good shape, possibly even convert back.
Approving this for merge.
rerequested review since there were quite big changes |
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 will have more time soon to both thoroughly test and review this more in depth but, at first glance visual review this looks like it will work just fine. I don't see anything in this WIP that stands out to be not great. Excellent work @shoshta73! Thanks. I have a bunch to contribute as soon as I'm able to, mainly enhancing the files we already have. I really appreciate your work. Approving this set of WIP commits.
This is still work in progress,
requirements for this to be merged: