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 Tuplable #49

Merged
merged 6 commits into from
Jun 1, 2021
Merged

add Tuplable #49

merged 6 commits into from
Jun 1, 2021

Conversation

carllerche
Copy link
Member

A proposal for handling tuples. If this looks like a good direction, I will finish it up (docs / tests). I would also probably change Value::Unit -> Value::Tuplable and add Value::Some and Value::None.

@carllerche carllerche requested review from hawkw and taiki-e May 23, 2021 04:59
@carllerche carllerche marked this pull request as draft May 23, 2021 04:59
@taiki-e
Copy link
Member

taiki-e commented May 25, 2021

  • I feel using the tuple-specific trait (Tuplable) is good than other known approaches (unnamed Structable that allows "unnamed struct with named fields", Listable that lose some information, etc.).
  • For now, I tend to prefer to leave Value::Unit. It can be handled more easily than an empty Tuplable. Also, I feel it is not clear what Tuplable is considered unit. If we have a method that checks for is unit or not, I'm ok with remove Value::Unit.
  • I'm in favor of representing Option as Value::Some and Value::None.

@carllerche carllerche marked this pull request as ready for review May 27, 2021 20:47
@carllerche
Copy link
Member Author

@taiki-e this should be ready for review. I did not fully update valuable-serde to handle all tuples, but did fix it for Unit.

A follow up PR will be needed to add Value::Some(...) and Value::None.

@carllerche carllerche merged commit b1e12a9 into master Jun 1, 2021
@taiki-e taiki-e deleted the tuplable branch June 1, 2021 23:25
This was referenced Jun 2, 2021
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.

2 participants