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 support for i128 / u128 #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

Mingun
Copy link
Contributor

@Mingun Mingun commented Aug 5, 2023

Because MSRV is 1.56 which has support of 128-bit integers, serde_if_integer128 hack is not needed.

This is breaking change because Token enum is not marked as non_exhaustive and it is changed.

Closes #18

@Mingun
Copy link
Contributor Author

Mingun commented Aug 7, 2023

@dtolnay, is it possible to merge this (and #5 in its original, breaking manner) and release a version 2? I think, that support of 128-bit integer are good enough reason to release 2.0?

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

I think I would not want to do 2.x over this.

If there is a more fully featured serde testing library that people use in the meantime, that is all right with me. Serde_test is not special outside of being the crate that serde's test suite has used. But there is not a particular ecosystem value to everybody sticking to the same library as serde's test suite.

Some other things that should be evaluated for a 2.x:

  • Order-insensitive comparison, such as being able to test data structures containing HashMap/HashSet, which is not possible today.
  • Do skip_field calls need to be represented.
  • Is human_readable integrated in the right way. This is something that was tacked on substantially later than 1.0.
  • Examine all the current users of serde_test on crates.io and any lessons learned from that usage.
  • Any relevant open issues on the serde repo.

@Mingun
Copy link
Contributor Author

Mingun commented Aug 8, 2023

This support is just needed in order to make some comprehensive tests in serde test_suite. Right now I have to avoid testing support for 128-bit integer, which, given that it is supported in serde only partially, makes it much more difficult to identify places where it is absent and what can affect

Some other things that should be evaluated for a 2.x:

That's really needed to do at once in one release? It seems to me that it is enough to include that features that's ready, in order of their readiness

@Mingun
Copy link
Contributor Author

Mingun commented Aug 28, 2023

Because since serde-rs/serde#2600 the 128-bit integers are not gated anymore it would be useful to test they support and serde_test should support that for that purpose. @dtolnay, maybe you reconsider to merge this and release a new version of serde_test? It is hardly ever that anyone will notice that this change is a breaking change, because there is no reasons to match on Token (new variants is the only breaking change here), so even your non-standard understanding of crates versioning wouldn't suffer.

Because MSRV is 1.56 which has support of 128-bit integers, serde_if_integer128 hack is not needed
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.

Add Token::I128 and Token::U128 to serde_test
2 participants