Skip to content

Make LengthPrefixedVec generic - #410

Merged
caelunshun merged 7 commits into
feather-rs:mainfrom
kirawi:generic
Apr 11, 2021
Merged

caelunshun merged 7 commits into
feather-rs:mainfrom
kirawi:generic

Conversation

@kirawi

@kirawi kirawi commented Apr 9, 2021

Copy link
Copy Markdown
Contributor

Make LengthPrefixedVec generic

Status

  • Ready

Description

Made it generic and added typedefs for compatibility with existing code, and removed ShortPrefixedVec implementation. As per this conversation. Honestly, I'm not sure if performance will suffer, but I hope not.

Checklist

  • Ran cargo fmt, cargo clippy, cargo build --release and cargo test and fixed any generated errors!
  • Removed unnecessary commented out code
  • Used specific traces (if you trace actions please specify the cause i.e. the player)

Comment thread feather/protocol/src/io.rs Outdated
@kirawi kirawi mentioned this pull request Apr 9, 2021
6 tasks
Comment thread feather/protocol/src/io.rs Outdated
@Schuwi

Schuwi commented Apr 10, 2021

Copy link
Copy Markdown
Member

Looks very good overall! :)

@kirawi

kirawi commented Apr 10, 2021

Copy link
Copy Markdown
Contributor Author

I made the trait return a result, and all other functions simply unwrap it. I'll probably change it to have proper error propagation in the future.

@kirawi

kirawi commented Apr 10, 2021

Copy link
Copy Markdown
Contributor Author

Okay, I think it should be good enough for now. Ready for review. In the future, the error handling would likely need to be overhauled since my work was blindly done, and I expect it may eventually unwrap an error when we don't want it to. However, I would consider it technical debt for getting it up and running. For such a future PR, here's an idea by @Schuwi on possibly handling it.

@caelunshun caelunshun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@caelunshun
caelunshun merged commit 679bc6e into feather-rs:main Apr 11, 2021
@kirawi
kirawi deleted the generic branch April 11, 2021 03:15
@kirawi
kirawi restored the generic branch April 11, 2021 18:15
@kirawi
kirawi deleted the generic branch April 12, 2021 17:55
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

Successfully merging this pull request may close these issues.

3 participants