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

Fixed behavior of default fn in dump and dumps #52

Merged
merged 2 commits into from May 6, 2022

Conversation

alexreg
Copy link
Contributor

@alexreg alexreg commented Oct 6, 2021

Fixes #48.

@alexreg alexreg changed the title Fixed behavior of default fn in dump and `dumps. Fixed behavior of default fn in dump and dumps Oct 6, 2021
@alexreg alexreg force-pushed the fix-default-fn branch 2 times, most recently from ea9ace9 to e644660 Compare October 6, 2021 20:14
@dpranke
Copy link
Owner

dpranke commented Oct 6, 2021

Thanks for the fix! It looks good. Can you add a test for this (and/or update the existing test) to tests/lib_test.py?

Also, same question as in #51 (comment) about whether you'd be willing to sign the CLA?

@alexreg
Copy link
Contributor Author

alexreg commented Oct 6, 2021

Yep, certainly. Wanted to check you agreed this behaviour is the correct one first... will write the test shortly. CLA signed.

@alexreg
Copy link
Contributor Author

alexreg commented Oct 6, 2021

I updated the test_default test so it passes now. The behaviour after this fix is that json5.dump conforms with both its own documentation and the behaviour of json.dump with respect to the default function. Whether you consider this a breaking change or merely a bug fix, and how you adjust version numbers, I leave up to you!

Copy link
Owner

@dpranke dpranke left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good.

This appears to be a breaking change (in the sense that the encoding of the string will change, for example), but it seems hard to argue that the existing implementation was right, since it clearly could produce invalid json5. I think I'll just note this in the release notes and see if anyone hits issues with this.

@alexreg
Copy link
Contributor Author

alexreg commented Oct 11, 2021

@dpranke Yeah, that logic makes sense to me too. i doubt you'll get too many complaints, if any at all.

@dpranke dpranke merged commit 50898a4 into dpranke:main May 6, 2022
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.

Control formatting with dump / dumps
2 participants