-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
File upload - simplify a bit with gloo::file::FileList::from #3685
Conversation
Size Comparison
✅ None of the examples has changed their size significantly. |
examples/file_upload/src/main.rs
Outdated
self.files.push(file); | ||
self.readers.remove(&name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you flip these two lines, you should be able to avoid the clone above. It doesn't really matter though
Oh smart, will do. I didn' want to change the logic of why they kept it that way, but now without anything else happening, it does seem silly.
In a real usage with error handling, I might not want to pull the reader out till it was completed successfully, but as this doens't have that, cloning it is just a waste (and someone will handle their own stuff for that for their real usage). And debatable you should pull it out, do stuff, and handle your error accordingly. But all in all, more thinking perhaps than "just stick in the Vec" requires, heh.
…On Sun, Jul 21, 2024 at 05:15 Elina ***@***.***> wrote:
***@***.**** approved this pull request.
------------------------------
In examples/file_upload/src/main.rs
<#3685 (comment)>:
> + self.files.push(file);
+ self.readers.remove(&name);
If you flip these two lines, you should be able to avoid the clone above.
It doesn't really matter though
—
Reply to this email directly, view it on GitHub
<#3685 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAAANNNXYUPW6S7UXONQVLZNOQ7PAVCNFSM6AAAAABLFUUTHWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCOJQGMYTKMRRGM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks! If you merge latest changes from master, the Rust CI should be green
# Conflicts: # Cargo.lock
…k#3685) * simplify a little with gloo::file::FileList::from * a little less repetition * trim the web_sys feature flags a bit * drop the clone * more golf * fmt --------- Co-authored-by: Elina <[email protected]>
Description
gloo::file::FileList::from
can do some of the conversion for us, removing the need for an auxiliary fn. This leads to trimming down the callbacks a bit. And hey, we have this FileDetails, why not just use it instead of passing the vars separately?Checklist
Example tests?