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 the Tag trait #30

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

Conversation

sebastianblunt
Copy link

This allows people to write custom implementations of the Tag trait
rather than having to rely on format! or similar and passing in tags as
str/String.

The Tag trait has a blanket implementation for AsRef so this change
should be backwards compatible.

I don't have any strong need to include TagTuple in the upstream library,
it's mostly just as an example usage. The primary purpose is just to be able
to have tags that don't rely on being formatted before being passed to this
library.

This allows people to write custom implementations of the Tag trait
rather than having to rely on format! or similar and passing in tags as
str/String.

The Tag trait has a blanket implementation for AsRef<str> so this change
should be backwards compatible.
@sebastianblunt
Copy link
Author

sebastianblunt commented Dec 27, 2019

As is this doesn't let people create Tag trait objects because of the generic writer in write_tag.

I could change it in a few ways

  • Move W to the Tag trait (pub trait Tag<W: io::Write>) - this would add some verbosity to the types in this library as now every function will need to be generic over W, T where W: io::Write, T: Tag<W> and similarly any places using Tag trait objects will now also have to be generic over W.
  • Make the write_tag function take a trait object to the writer, (fn write_tag(&self, w: &mut dyn io::Write)). Theoretically it should be a very small performance loss, but it seems like the compiler is able to optimize away the dynamic dispatch in release mode.
  • Specify the exact writer type. It's only Vec<u8> right now. However doing this would cause changing that type to be a breaking change for any crates that have custom Tag types.
  • Do nothing. Library consumers wishing to pass multiple types of tags in at once will need to define an enum covering all the different types. Could write a macro let people easily create such enums.

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.

1 participant