serenity icon indicating copy to clipboard operation
serenity copied to clipboard

[HTTP] Remove unnecessary clones by passing around owned data

Open GnomedDev opened this issue 3 years ago • 2 comments

A lot of the methods on Request and AttachmentType were only given references or mutable references to their data, instead of taking owned data. This was unnecessary and caused a lot of cloning. This PR fixes this

GnomedDev avatar Jul 26 '22 15:07 GnomedDev

Looking at the tests that failed, they are related to the AttachmentType From impl changing, but taking a second look it seems to be quite ambiguous anyway, should I remove these conversions and make it explicit for the user?

GnomedDev avatar Jul 27 '22 00:07 GnomedDev

Clean diff here: https://github.com/serenity-rs/serenity/compare/5edbf0b3a720c528394a5d88325cb2ec804ae31b...GnomedDev:serenity:cleanup-http

Currently blocked on @GnomedDev reviewing https://github.com/GnomedDev/serenity/pull/1 I believe?

kangalio avatar Sep 20 '22 23:09 kangalio

Sorry for the close and reopen, was accidental.

I've just removed most of the public AttachmentType changes, apart from changing the Cow<'a, [u8]> to Vec<u8> as previously it was useless (but the From implementation still uses Cow, someone else can bikeshed them).

GnomedDev avatar Sep 23 '22 17:09 GnomedDev

There is harm, it is misleading currently as if it is passed through it goes into impl Into<Cow<'static, [u8]>> which means that the Cow is useless as-is.

GnomedDev avatar Sep 23 '22 23:09 GnomedDev

Not sure what you mean, could you give an example?

mkrasnitski avatar Sep 23 '22 23:09 mkrasnitski

@GnomedDev now that #2163 has been merged, could you rebase this on top and remove the attachments changes?

mkrasnitski avatar Oct 09 '22 02:10 mkrasnitski