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

WIP: Refactor JSON parser/writer, add YAML support #132

Closed
wants to merge 1 commit into from
Closed

WIP: Refactor JSON parser/writer, add YAML support #132

wants to merge 1 commit into from

Conversation

ianling
Copy link
Collaborator

@ianling ianling commented Apr 11, 2022

Refactors the JSON parser/writer so it no longer uses reflect. The new approach is incomparably more maintainable and less error-prone, and requires substantially fewer unit tests.

Adds YAML support.

Using Go's built-in JSON functionality gives us YAML support for free using https://pkg.go.dev/sigs.k8s.io/yaml

There are a number of breaking API changes here (I changed some of the structs to align more with the spec). Maybe once full support for all three of these file types is finished in this PR (plus RDF, which is already in the module), this warrants an API-breaking 1.0.0 release of these tools?

I'm investigating XLS support as well, which, if added, would mean that this module has full support for all of the formats listed in the spec.

@ianling ianling changed the title Refactor JSON parser/writer, add YAML and XML support Refactor JSON parser/writer, add YAML support Apr 11, 2022
@ianling ianling marked this pull request as ready for review April 11, 2022 20:36
@ianling
Copy link
Collaborator Author

ianling commented Apr 11, 2022

(title of PR originally mentioned XML because I got this mixed up with CycloneDX, where XML is already a supported format)

@CatalinStratu
Copy link
Contributor

Hi, @ianling, it looks great, we enjoy new contributors.

I believe that this PR should be divided into smaller PRs to make it easier to code review.

@ianling
Copy link
Collaborator Author

ianling commented Apr 11, 2022

I am not sure how to break it into smaller chunks unfortunately; in order to maintain the same functionality in the JSON parser/writer, the entire struct refactor needs to be added at once. If I do it piecemeal, I have to go back and make changes to the old JSON parser/writer which would then be removed in my later commits.

In other words, if I break it into smaller commits, there will be commits where the tools are broken.

I can move the YAML commit into a different PR, but the changes for that are contained entirely in the yaml folder.

@CatalinStratu
Copy link
Contributor

In this PR, issue #124 is fixed, but there is a new issue
image

@CatalinStratu
Copy link
Contributor

I am not sure how to break it into smaller chunks unfortunately; in order to maintain the same functionality in the JSON parser/writer, the entire struct refactor needs to be added at once. If I do it piecemeal, I have to go back and make changes to the old JSON parser/writer which would then be removed in my later commits.

In other words, if I break it into smaller commits, there will be commits where the tools are broken.

I can move the YAML commit into a different PR, but the changes for that are contained entirely in the yaml folder.

@ianling ,yes, it is a good idea to move YAML to PR separately

@ianling
Copy link
Collaborator Author

ianling commented Apr 11, 2022

I missed running those examples during my testing, I will do that and report back (and fix bugs I introduced). Thanks for taking a look.

@ianling ianling marked this pull request as draft April 11, 2022 20:54
@ianling ianling changed the title Refactor JSON parser/writer, add YAML support WIP: Refactor JSON parser/writer, add YAML support Apr 11, 2022
Signed-off-by: Ian Ling <ian@iancaling.com>
@ianling
Copy link
Collaborator Author

ianling commented Apr 11, 2022

Closing this PR in favor of #133 and #134

@ianling ianling closed this Apr 11, 2022
@ianling ianling deleted the yaml branch April 11, 2022 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants