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

explicitly specify error set of std.json.stringify #16323

Merged
merged 1 commit into from
Jul 10, 2023

Conversation

Techatrix
Copy link
Contributor

fixes #15859
This change wouldn't be necessary if #2971 was implemented, but until that happens, this will make using std.json.stringify on recursive structures easier to deal with.

@Techatrix Techatrix force-pushed the explicit-json-stringify branch from fbf2e19 to 4f17351 Compare July 4, 2023 19:10
@andrewrk andrewrk requested a review from thejoshwolfe July 7, 2023 18:55
@andrewrk
Copy link
Member

andrewrk commented Jul 7, 2023

#2971 is solved already, it just needs test coverage in order to be closed.

@thejoshwolfe
Copy link
Contributor

This PR reverts a feature implemented explicitly by #15172 , but there was no link to a use case in that original PR, so it's hard to tell the impact of removing it. Can @mateusz834 weigh in on why custom errors from jsonStringify() were originally implemented?

@mateusz834
Copy link
Contributor

I've had a custom stringifier, that required an allocator, and it was annoying that it worked fine while writing to ArrayList.Writer(), but not to other writers that didn't return the OutOfMemory.

@thejoshwolfe
Copy link
Contributor

@Techatrix do you think there's a way to use some kind of generic programming to enable both use cases? It seems a little tacky to throw error.OutOfMemory into the error set unconditionally, but technically that would address the original use case.

Or @mateusz834 is this no longer a required use case? Would it break anything to drop support for the original feature?

@Techatrix
Copy link
Contributor Author

do you think there's a way to use some kind of generic programming to enable both use cases?

Coincidentally this is exactly what I was looking into right now. The tres library uses a generic function to resolve the error set of its json parse function that could also work for stringify.

@mateusz834
Copy link
Contributor

Or @mateusz834 is this no longer a required use case? Would it break anything to drop support for the original feature?

It is fine to break this.

Copy link
Contributor

@thejoshwolfe thejoshwolfe left a comment

Choose a reason for hiding this comment

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

It is fine to break this.

In that case, I don't think we need to over-complicate this. 🤷 This PR seems good as-is.

@andrewrk andrewrk merged commit 3bf0b8e into ziglang:master Jul 10, 2023
@Techatrix Techatrix deleted the explicit-json-stringify branch September 6, 2023 21:02
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.

Issue to stringify nested structure
4 participants