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

Add json handlers #93

Merged
merged 6 commits into from May 16, 2022
Merged

Add json handlers #93

merged 6 commits into from May 16, 2022

Conversation

jukie
Copy link
Contributor

@jukie jukie commented May 15, 2022

Closes #94
This adds two functions that implement JSON.Unmarshaler and JSON.Marshaler interface to allow for parsing to/from json.

@hashicorp-cla
Copy link

hashicorp-cla commented May 15, 2022

CLA assistant check
All committers have signed the CLA.

version_test.go Outdated
Comment on lines 438 to 447
{"1.2.3", false},
{"1.2.0-x.Y.0+metadata", false},
{"1.2.0-x.Y.0+metadata-width-hypen", false},
{"1.2.3-rc1-with-hypen", false},
{"1.2.3.4", false},
{"1.2.0.4-x.Y.0+metadata", false},
{"1.2.0.4-x.Y.0+metadata-width-hypen", false},
{"1.2.0-X-1.2.0+metadata~dist", false},
{"1.2.3.4-rc1-with-hypen", false},
{"1.2.3.4", false},
Copy link
Contributor Author

@jukie jukie May 15, 2022

Choose a reason for hiding this comment

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

Not sure what values would make sense to force a failure here.

Copy link

Choose a reason for hiding this comment

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

Hi @jukie,
Thanks for submitting this PR. I don't see much benefit in negative tests here because errors are only returned by code that's not specifically under test by this test function (the standard library json package and the parsing by NewVersion()). The code that really needs to be tested in your implementation is the transfer of data from the temporary struct to the receiver, and I think the tests you already have cover that logic.

@mdeggies mdeggies requested review from a team, shore and mdeggies and removed request for a team May 16, 2022 17:35
Copy link

@shore shore left a comment

Choose a reason for hiding this comment

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

This looks pretty good overall. I think there are some hypen/hyphen typos, and the test suite doesn't pass when I run it locally.

@jukie
Copy link
Contributor Author

jukie commented May 16, 2022

@shore looks like I pushed an incomplete commit. Sorry about that, should be actually ready now.
Also fixed the hypen typos.

@jukie jukie requested a review from shore May 16, 2022 23:08
Copy link

@shore shore left a comment

Choose a reason for hiding this comment

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

👍

@shore shore merged commit 82485a6 into hashicorp:main May 16, 2022
@jukie
Copy link
Contributor Author

jukie commented May 17, 2022

@shore any chance I could also request for a release cut in the next couple weeks?

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.

Support json marshaling/unmarshaling
3 participants