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

Naming Bikeshed: Response::into_{json,xml}_body #1957

Open
analogrelay opened this issue Dec 13, 2024 · 1 comment
Open

Naming Bikeshed: Response::into_{json,xml}_body #1957

analogrelay opened this issue Dec 13, 2024 · 1 comment

Comments

@analogrelay
Copy link
Member

#1954 introduces two new methods on Response:

  • Response::<T>::into_json_body<U: DeserializeOwned>(self) -> crate::Result<U>
  • Response::<T>::into_xml_body<U: DeserializeOwned>(self) -> crate::Result<U>

The purpose of these methods is to consume the Response, stream the entire body, and deserialize it to the provided model type U, using either JSON or XML serde deserialization.

The method name isn't ideal, into properly implies consuming the Response to produce a new type, but the {json,xml}_body implies that it's "returning" JSON or XML somehow, when actually it's using JSON/XML deserialization to produce a model type.

However, we also have methods into_raw_body and into_body for returning a raw ResponseBody and a T: Model, which make more sense. Both are consuming the Response to produce a (potentially "raw") representation of the body. The into_{json,xml}_body methods align with the convention established by those methods.

We decided to move forward without resolving this naming conflict, so this issue exists to have a place to revisit it prior to a GA release, after which name changes would be much more costly.

Some potential ideas for discussion:

  • into_body_from_{json,xml} is more accurate, but verbose
  • body_from_{json,xml} loses the into_ prefix, which has meaning in Rust
  • parse_{json,xml} is more accurate, relatively terse, but does still lose the into_ prefix
  • into_{json,xml}_model may be clearer, since it indicates the model is defined by JSON or XML rather than implying it returns the "body". We could similarly rename into_body to into_model 🤔

cc @heaths

@heaths
Copy link
Member

heaths commented Dec 13, 2024

Hmm, hadn't considered parse_ but I think that almost works; however, std parse functions all borrow so we do lose the intent of the into_ ownership transfer. What about parse_{json,xml}_into()? Then again, that almost seems to imply into_body() should instead be into()...but is that so bad, I wonder now? I mean, a model could, technically, pull from headers to you're consuming more than just the body. into_raw() works as well.

The 4th bullet are good ideas as well, but I'm currently leaning toward brevity above e.g.,

  • into(self) -> Model...or even parse_into(self)?
  • into_raw(self) -> ResponseBody
  • parse_json_into<T>(self) -> Model (from JSON)
  • parse_xml_into<T>(self) -> Model (from XML)

Though, writing that out, prefaces are all over the place. Maybe your fourth bullet is better. @JeffreyRichter @RickWinter what are your thoughts?

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

No branches or pull requests

2 participants