http icon indicating copy to clipboard operation
http copied to clipboard

Should `Version` impl `Display`?

Open ADD-SP opened this issue 4 months ago • 2 comments

I'm a little surprised that Version doesn't impl std::fmt::Display when I formatting the log, so I use Debug for now.

I searched issues but didn't find relevant topic.

Is adding the Display impl ok? Are there some concerns I missed?

ADD-SP avatar Aug 12 '25 01:08 ADD-SP

I feel like I've answered this before, but now I cannot find it in search. Anyways, the reason is hasn't existed is, what does the Display mean for Version? The only one that serializes the version is HTTP/1, 2 and 3 don't bother.

seanmonstar avatar Aug 12 '25 17:08 seanmonstar

For me, I don't think the Debug harms my use case, so I just want to get your thought on Display or not Display.

It makes sense not to impl Display for Version because of the its serializations are different for HTTP/1 and HTTP/2, let alone we might also need to impl FromStr after the Display.

However, the HeaderName has implemented Display, the header name is a simple string in HTTP/1, but it might be Huffman encoded binary in HTTP/2. Looks like the serialization differences are not the strong reason to make this decision.

I found #249 that added the Display impl to HeaderName, and its description looks better than the serialization reason.

"Display is not implemented for Version is because we don't find its use case except logging, so Debug is enough."

I would say this reason is better to me. Could you share you thought on this topic (Display or not Display on a type)?

ADD-SP avatar Aug 13 '25 01:08 ADD-SP