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

Encode any JSON key to string, closes #14305 #14309

Merged
merged 2 commits into from
Mar 5, 2025
Merged

Encode any JSON key to string, closes #14305 #14309

merged 2 commits into from
Mar 5, 2025

Conversation

josevalim
Copy link
Member

@michalmuskala
Copy link
Member

The main issue with the Jason API around this was error handling for the tuple-returning variant. In particular initially I didn't rescue the String.Chars protocol not implemented errors, and this was always left like that for fear of breaking compatibility. Perhaps the Elixir API can do this well.
That said, I see it doesn't seem to be rescuing the protocol-not-implemented errors for JSON.Encode either, so perhaps that's actually the better behaviour.

@josevalim
Copy link
Member Author

We don't provide the tuple variant exactly because of issues like these. :) So I will go ahead, merge this, and ship a new version! Thank you!

Copy link
Member

@whatyouhide whatyouhide left a comment

Choose a reason for hiding this comment

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

This is nice! Maybe some comments in the code for why we do this would help but code-wise it looks spot on

@josevalim josevalim merged commit 65ae623 into main Mar 5, 2025
18 of 20 checks passed
@josevalim josevalim deleted the jv-json-key branch March 5, 2025 08:47
@josevalim
Copy link
Member Author

💚 💙 💜 💛 ❤️

josevalim added a commit that referenced this pull request Mar 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants