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

feat: csv support for conversiont to json #48

Merged
merged 46 commits into from
Dec 11, 2024
Merged

feat: csv support for conversiont to json #48

merged 46 commits into from
Dec 11, 2024

Conversation

shoshta73
Copy link
Collaborator

@shoshta73 shoshta73 commented Nov 29, 2024

This is still work in progress,

requirements for this to be merged:

  • Converter app that will comunicate via stdin/stdout/stderr
  • Tests for new Loader and Converter classes
  • Update documentation
  • cleanup assertions in source code

@shoshta73 shoshta73 self-assigned this Nov 29, 2024
@shoshta73 shoshta73 requested a review from gbowne1 November 29, 2024 01:03
@shoshta73 shoshta73 added the enhancement New feature or request label Nov 29, 2024
@shoshta73
Copy link
Collaborator Author

@gbowne1 if you need/want anything else added to this pr feel please add it to the list in opening comment

Copy link
Owner

@gbowne1 gbowne1 left a 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.

@shoshta73
Copy link
Collaborator Author

@gbowne1 i didnt realize i was developing under windows virtaul machine, but i noticed long import times(under low resources in vm), so i made these 3 commits fe4abfa, 39c3125, 31c6880

@shoshta73 shoshta73 requested a review from gbowne1 December 1, 2024 01:51
@shoshta73
Copy link
Collaborator Author

shoshta73 commented Dec 1, 2024

rerequested review since there were quite big changes

Copy link
Owner

@gbowne1 gbowne1 left a 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.

@shoshta73 shoshta73 changed the title WIP: feat: csv support for conversiont to json feat: csv support for conversiont to json Dec 11, 2024
@shoshta73 shoshta73 merged commit b542c7c into master Dec 11, 2024
31 checks passed
@github-actions github-actions bot deleted the feat/csv branch December 11, 2024 05:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants