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

Ensure that ujson.Value behaves properly as a direct target of reading/writing #436

Merged
merged 2 commits into from
Jan 31, 2023

Conversation

lihaoyi
Copy link
Member

@lihaoyi lihaoyi commented Jan 31, 2023

The basic issue is that the StringVisitor/StringReader used for processing map keys when building JSON ASTs (

def visitKey(index: Int) = upickle.core.StringVisitor
) was not sufficiently forgiving in accepting non-string types, which can be given to it when serializing a map with non-string keys.

This "loosening" of the input handling is in line with the general trend throughout uPickle, similar to how in 2.0.0 we allowed #385 we allowed numeric Scala types to accept JSON strings as input

This PR adds it to the test suite conversion matrix, so this code path is additionally exercised for every value in our test suite, and fixed another bug uncovered that crashed when directly writing Infinity and -Infinity to ujson.Value

Fixes #433

…g/writing and add it to the test suite matrix for all tests
@lihaoyi lihaoyi requested review from lefou and lolgab January 31, 2023 05:46
Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. The binary breakage is ok here, but it's interesting, that it only breaks for Scala.js.

@lefou
Copy link
Member

lefou commented Jan 31, 2023

Looks reasonable to me. The binary breakage is ok here, but it's interesting, that it only breaks for Scala.js.

Oh, it runs without --keep-going, so it probably also fails for all other platforms.

@lihaoyi
Copy link
Member Author

lihaoyi commented Jan 31, 2023

We're going to release 3.0.0 next anyway right? which would mean binary compat breakage doesn't matter?

@lefou
Copy link
Member

lefou commented Jan 31, 2023

We're going to release 3.0.0 next anyway right? which would mean binary compat breakage doesn't matter?

Exactly. The whole point about the 3.0.0-M1 pre-release was to get some binary breaking things in.

@lefou lefou added this to the after 3.0.0-M1 milestone Jan 31, 2023
@lefou lefou merged commit f7b61ce into main Jan 31, 2023
@lefou lefou deleted the fix-ujson-value branch January 31, 2023 15:04
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.

Using writeJs with Map with type other than String produces error
2 participants