Replace new `Request` generic param with const parameter #2646

mkrasnitski · 2023-12-03T18:37:13Z

Follow-up to #2624

We can rework the new Request generics to avoid having to use a turbofish everywhere by using const parameters which can be inferred easier. Unfortunately, a concrete choice for N must still be made when calling Request::new.

Slight behavioral change - before, setting params to Some(vec![]) would still append a question mark to the end of the url. Now, we check if the params array is_empty instead of is_some, so the question mark won't be appended if the params list is empty (which is IMO more correct).

Once this gets merged it's probably good to squash with #2624 so that future rebases of next don't become painful with back-to-back commits causing conflicts.

GnomedDev

N is really non-descriptive and requires a comment to explain. Can it be changed to MAX_PARAMS?

src/http/request.rs

src/http/client.rs

Allows getting rid of the otherwise-required turbofish

mkrasnitski · 2023-12-03T20:25:26Z

Unfortunately in this process of adding a params argument, Request::new had to become non-const, but considering that doing anything meaningful with the Request before sending it off (like setting a body and headers) can't be done const, I don't think there's any real downside here.

arqunis · 2023-12-03T23:46:47Z

src/http/request.rs

+    /// # Panics
+    /// Panics if the length of the slice exceeds the value of `MAX_PARAMS`.
+    pub fn params(mut self, params: &[(&'static str, String)]) -> Self {
+        self.params = params.try_into().expect("number of params exceeded capacity");


Suggested change

/// # Panics

/// Panics if the length of the slice exceeds the value of `MAX_PARAMS`.

pub fn params(mut self, params: &[(&'static str, String)]) -> Self {

self.params = params.try_into().expect("number of params exceeded capacity");

pub fn params(mut self, params: [(&'static str, String); MAX_PARAMS]) -> Self {

self.params = params;

The motivation here is that the original argument was of type Option<Vec<_>>. Converting a vec into an array requires calling .try_into().unwrap(). I guess the more explicit version is a better tradeoff.

…erenity-rs#2646) This avoids having to allocate to store fixed length (replaced with normal array) or fixed capacity (replaced with `ArrayVec`) collections as vectors for the purposes of putting them through the `Request` plumbing. Slight behavioral change - before, setting `params` to `Some(vec![])` would still append a question mark to the end of the url. Now, we check if the params array `is_empty` instead of `is_some`, so the question mark won't be appended if the params list is empty. Co-authored-by: Michael Krasnitski <[email protected]>

This avoids having to allocate to store fixed length (replaced with normal array) or fixed capacity (replaced with `ArrayVec`) collections as vectors for the purposes of putting them through the `Request` plumbing. Slight behavioral change - before, setting `params` to `Some(vec![])` would still append a question mark to the end of the url. Now, we check if the params array `is_empty` instead of `is_some`, so the question mark won't be appended if the params list is empty. Co-authored-by: Michael Krasnitski <[email protected]>