feat(http,model,util,validate)!: use `Cow<'a, [u8]>` for attachments #1923

itohatweb · 2022-09-17T14:10:50Z

Instead of taking &'a [u8] for attachments, which could cause lifetime issues when using functions like fn create_attachment<'a>() -> Attachment<'a>, Cow<'a, u8> can be used to support both creating attachments from owned and borrowed bytes.

This is a followup idea to: #1886

Instead of taking `&'a [u8]` for attachments, which could cause lifetime issues when using functions like `fn create_attachment<'a>() -> Attachment<'a>`, `Cow<'a, u8>` can be used to support both creating attachments from owned and borrowed bytes. This is a followup idea to: #1886

itohatweb · 2022-09-22T15:43:23Z

Why the direct commit without discussing?

vilgotf · 2022-09-22T15:46:54Z

I don't see why Cow is necessary?

Why the direct commit without discussing?

Wanted to run CI on my diff, could and maybe should've done it on a private branch (though a single commit is easily revertable).

itohatweb · 2022-09-22T15:51:40Z

Please revert that and request changes, if you want to use &[U8] instead.

vilgotf

This can be slightly modified to work directly with &'a [u8], see the diff I sent in https://discord.com/channels/745809834183753828/1020775082123071508

itohatweb · 2022-09-22T20:01:44Z

This can be slightly modified to work directly with &'a [u8], see the diff I sent in https://discord.com/channels/745809834183753828/1020775082123071508

I have chosen to use Cow since users might want to create attachments in a separate function. Using &[u8] would break this functionality.
Reference: https://discord.com/channels/745809834183753828/1020775082123071508/1022591932163162112

vilgotf

Should update examples to utilize &'static [u8] when possible

vilgotf · 2022-09-22T20:23:15Z

twilight-model/src/http/attachment.rs

@@ -21,18 +24,18 @@ use serde::{Deserialize, Serialize};
 /// .to_vec();


Shouldn't encourage needless allocations.

Suggested change

/// .to_vec();

vilgotf · 2022-09-22T20:23:26Z

twilight-model/src/http/attachment.rs

@@ -21,18 +24,18 @@ use serde::{Deserialize, Serialize};
 /// .to_vec();
 /// let id = 1;
 ///
-/// let mut attachment = Attachment::from_bytes(filename, file_content, id);
+/// let mut attachment = Attachment::from_bytes(filename, Cow::from(file_content), id);


Cow::from is not smart enough for &'static [u8] data (needed for it to work without .to_vec())

Suggested change

/// let mut attachment = Attachment::from_bytes(filename, Cow::from(file_content), id);

/// let mut attachment = Attachment::from_bytes(filename, Cow::Borrowed(file_content), id);

vilgotf · 2022-09-22T20:25:37Z

twilight-model/src/http/attachment.rs

    /// Create an attachment from a filename and bytes.
    ///
    /// # Examples
    ///
    /// Create an attachment with a grocery list named "grocerylist.txt":
    ///
    /// ```
+    /// use std::borrow::Cow;
    /// use twilight_model::http::attachment::Attachment;
    ///
    /// let filename = "grocerylist.txt".to_owned();
    /// let file_content = b"Apples\nGrapes\nLemonade".to_vec();


Suggested change

/// let file_content = b"Apples\nGrapes\nLemonade".to_vec();

/// let file_content = b"Apples\nGrapes\nLemonade";

vilgotf · 2022-09-22T20:25:45Z

twilight-model/src/http/attachment.rs

    /// use twilight_model::http::attachment::Attachment;
    ///
    /// let filename = "grocerylist.txt".to_owned();
    /// let file_content = b"Apples\nGrapes\nLemonade".to_vec();
    /// let id = 1;
    ///
-    /// let attachment = Attachment::from_bytes(filename, file_content, id);
+    /// let attachment = Attachment::from_bytes(filename, Cow::from(file_content), id);


Suggested change

/// let attachment = Attachment::from_bytes(filename, Cow::from(file_content), id);

/// let attachment = Attachment::from_bytes(filename, Cow::Borrowed(file_content), id);

vilgotf · 2022-09-22T20:26:18Z

twilight-model/src/http/interaction.rs

@@ -187,7 +187,7 @@ mod tests {
            data: Some(InteractionResponseData {
                attachments: Some(Vec::from([Attachment {
                    description: None,
-                    file: "file data".into(),
+                    file: "file data".as_bytes().into(),


Suggested change

file: "file data".as_bytes().into(),

file: Cow::Borrowed(b"file data"),

zeylahellyer · 2023-02-04T17:31:09Z

Triage: deferring updating + merging to the major version after this next one, 0.16

github-actions bot added c-http Affects the http crate c-model Affects the model crate c-util Affects the util crate c-validate Affects the validate crate m-breaking change Breaks the public API. t-feature Addition of a new feature labels Sep 17, 2022

itohatweb added 2 commits September 17, 2022 16:19

fix docs

a93eda3

fix wrong path

5d23287

7596ff approved these changes Sep 17, 2022

View reviewed changes

vilgotf approved these changes Sep 22, 2022

View reviewed changes

vilgotf force-pushed the itohatweb/feat/attachment-cow branch from cb5703f to 5d23287 Compare September 22, 2022 15:52

vilgotf requested changes Sep 22, 2022

View reviewed changes

zeylahellyer added the w-do-not-merge PR is blocked or deferred label Feb 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(http,model,util,validate)!: use `Cow<'a, [u8]>` for attachments #1923

feat(http,model,util,validate)!: use `Cow<'a, [u8]>` for attachments #1923

itohatweb commented Sep 17, 2022

itohatweb commented Sep 22, 2022

vilgotf commented Sep 22, 2022

itohatweb commented Sep 22, 2022

vilgotf left a comment

itohatweb commented Sep 22, 2022

vilgotf left a comment

vilgotf Sep 22, 2022

vilgotf Sep 22, 2022

vilgotf Sep 22, 2022

vilgotf Sep 22, 2022

vilgotf Sep 22, 2022

zeylahellyer commented Feb 4, 2023 •

edited

Loading

		@@ -21,18 +24,18 @@ use serde::{Deserialize, Serialize};
		/// .to_vec();

	/// let mut attachment = Attachment::from_bytes(filename, Cow::from(file_content), id);
	/// let mut attachment = Attachment::from_bytes(filename, Cow::Borrowed(file_content), id);

	/// let file_content = b"Apples\nGrapes\nLemonade".to_vec();
	/// let file_content = b"Apples\nGrapes\nLemonade";

	/// let attachment = Attachment::from_bytes(filename, Cow::from(file_content), id);
	/// let attachment = Attachment::from_bytes(filename, Cow::Borrowed(file_content), id);

	file: "file data".as_bytes().into(),
	file: Cow::Borrowed(b"file data"),

feat(http,model,util,validate)!: use Cow<'a, [u8]> for attachments #1923

Are you sure you want to change the base?

feat(http,model,util,validate)!: use Cow<'a, [u8]> for attachments #1923

Conversation

itohatweb commented Sep 17, 2022

itohatweb commented Sep 22, 2022

vilgotf commented Sep 22, 2022

itohatweb commented Sep 22, 2022

vilgotf left a comment

Choose a reason for hiding this comment

itohatweb commented Sep 22, 2022

vilgotf left a comment

Choose a reason for hiding this comment

vilgotf Sep 22, 2022

Choose a reason for hiding this comment

vilgotf Sep 22, 2022

Choose a reason for hiding this comment

vilgotf Sep 22, 2022

Choose a reason for hiding this comment

vilgotf Sep 22, 2022

Choose a reason for hiding this comment

vilgotf Sep 22, 2022

Choose a reason for hiding this comment

zeylahellyer commented Feb 4, 2023 • edited Loading

feat(http,model,util,validate)!: use `Cow<'a, [u8]>` for attachments #1923

feat(http,model,util,validate)!: use `Cow<'a, [u8]>` for attachments #1923

zeylahellyer commented Feb 4, 2023 •

edited

Loading