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

expose default options and serializers #310

Merged
merged 18 commits into from Apr 16, 2020

Conversation

@ZoeyR
Copy link
Collaborator

ZoeyR commented Mar 19, 2020

This is a very early PR that explores if its possible to expose the Serializer and Deserializer in a way that would allow us to expand options in the future.

Basic Design

  • expose the DefaultOptions and OptionsExt traits publicly.
  • expose Serializer and Deserializer publicly.
  • keep all other traits and structs Compound, Options private

I believe this would allow us to implement any future improvements in a backwards compatible manner. OptionsExt can't be implemented by 3rd party crates since it is sealed, so adding new methods should be fine.

Remarks

  • A custom ByteOrder trait was created that provides a thin wrapper over the byteorder trait.
    This enables us to add features to our endian behavior in the future without breaking the public API. e.g. making varint respect the endian choices.

Unanswered Questions

  • How should this be integrated with the current config() API?
  • Who (if anyone) is using the non-public SerializerAcceptor trait, and does this make that obsolete?

Tagging @jdm since (I think?) you would have perspective if servo is using these non-public APIs
Tagging @dtolnay since you had significant input on the current config implementation

@ZoeyR ZoeyR requested review from jdm and dtolnay Mar 19, 2020
@ZoeyR ZoeyR marked this pull request as ready for review Mar 25, 2020
ZoeyR added 7 commits Mar 19, 2020
@ZoeyR ZoeyR force-pushed the ZoeyR:expose-serializer-deserializer branch from 76347f5 to 356452d Mar 25, 2020
@ZoeyR ZoeyR changed the title [WIP] expose default options and serializers expose default options and serializers Mar 25, 2020
ZoeyR added 3 commits Mar 25, 2020
@jdm
Copy link
Member

jdm commented Mar 26, 2020

Servo only relies on official public APIs to my knowledge.

ZoeyR added 2 commits Mar 26, 2020
@ZoeyR
Copy link
Collaborator Author

ZoeyR commented Mar 26, 2020

Great to hear! I realized this makes the unwieldy config_map macro obsolete. Previously the match statement would have exploded exponentially, but now we won't need it.

src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@VictorKoenders
Copy link

VictorKoenders commented Apr 1, 2020

One of the things I dislike is that a caller has to import both DefaultOptions and OptionsExt, which might cause for confusion. This could be improved in two ways:

  • Make defaults for multiple combinations (e.g. BigEndianNoLimit, LittleEndianBounded)
  • Duplicate all methods on DefaultOptions so the trait is not always needed (might cause conflicts?)

Note: a lot of crates do this, it seems acceptable to just let this be.

ZoeyR added 5 commits Apr 15, 2020
@ZoeyR ZoeyR merged commit 0e35bd9 into servo:master Apr 16, 2020
8 checks passed
8 checks passed
Check (stable)
Details
Check (beta)
Details
Check (nightly)
Details
Check (1.18.0)
Details
Test (stable)
Details
Test (beta)
Details
Test (nightly)
Details
Lints
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.