-
Notifications
You must be signed in to change notification settings - Fork 888
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
Rename HTTPText to TextMap Propagator. #793
Rename HTTPText to TextMap Propagator. #793
Conversation
I would name this "TextMapPropagator" or "TextHeaderMapPropagator" to emphasize that it operates on a map of key/value pairs as opposed to simply returning/accepting a string (which might be the interface for the binary propagator). |
For the record, |
@carlosalberto @Oberon00 happy with |
@carlosalberto ping |
Renamed this to |
Would prefer now because before they were in the name otherwise this is a downgrade |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with this name.
I don't think we need to put an encoding in the name. If we want to have an ASCII-only propagator, we can do this strongly typed, but we don't have to. We could have a getAsciiTextMapPropagator and getTextMapPropagator on the Propagators class/interface both returning an instance of TextMapPropagator.
If we implement this strongly typed, I suggest writing the spec in such a way that you can implement AsciiTextMapPropagator as an empty interface deriving from TextMapPropagator.
@bogdandrutu Added a note with a To me, the most common case is still HTTP headers, so I'd vow to stick to ascii. For other encodings, the user should be able to (hopefully soon) use a Binary propagator instead ;) |
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
Fixes #317
HttpText
is renamed toText
for the Propagator component.I remember we barely discussing the possibility of renaming it to
AsciiText
instead (or similar), but wanted to try outText
first, as it's simpler, but I'm open to feedback ;)