Skip to content
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

refactor: framing_sv2 crate #903

Open
plebhash opened this issue May 9, 2024 · 7 comments · May be fixed by #1033 or #1049
Open

refactor: framing_sv2 crate #903

plebhash opened this issue May 9, 2024 · 7 comments · May be fixed by #1033 or #1049
Assignees
Labels
backlog Not necessary, but worth tracking refactor Implies refactoring code

Comments

@plebhash
Copy link
Collaborator

plebhash commented May 9, 2024

This task is an outcome of #845

While documenting the framing_sv2 crate, a lot of things stood out as needing to refactor.

The documentation effort was an immediate action, while a refactoring (which implies breaking API changes) is not so urgent priority, so for now we should leave this in the backlog for an appropriate moment in the future.

Below are the different items that I believe deserve to be rewritten on framing_sv2 crate.

rename framing2.rs

the character 2 in filename framing2.rs seems to make some kind of allusion to SV2, but this isn't following any pattern or convention in naming files.

so IMO this file should be renamed to framing.rs.

Frame trait removal

The Frame trait is unnecessary. It is never really used anywhere in the code as a Trait bound. The only usage of this trait is in the impl Frame blocks for Sv2Frame and HandShakeFrame. I even did a quick experiment where I commented out the Frame trait declaration and with small adaptations, I easily got the code to compile and run without errors.

The Frame trait makes some impositions into the APIs of Sv2Frame and HandShakeFrame structs by trying to unify them logically (they're both kinds of "frame"). But unfortunately, the resulting APIs are unintuitive, more complex, and dirtier than they need to be.

For example, the Sv2Frame::payload method is emitting a panic! when the API is used incorrectly. Ideally, the API should be built so that there's no footguns on when this function is used.

// self can be either serialized (it cointain an AsMut<[u8]> with the serialized data or
// deserialized it contain the rust type that represant the Sv2 message. If the type is
// deserialized self.paylos.is_some() is true. To get the serialized payload the inner type
// should be serialized and this function should never be used, cause is intended as a fast
// function that return a reference to an already serialized payload. For that the function
// panic.
fn payload(&'a mut self) -> &'a mut [u8] {
if let Some(serialized) = self.serialized.as_mut() {
&mut serialized.as_mut()[Header::SIZE..]
} else {
// panic here is the expected behaviour
panic!()
}
}

Since the Frame trait is never used anywhere else, there's only penalty for its existence (confusing APIs), for no profit.

I propose we remove Frame trait and refactor the APIs of Sv2Frame and HandShakeFrame so they become more concise and intuitive.

code organization

impl blocks should be written immediately after the declaration of the respective struct or enum.

We should avoid mixing impl blocks for different entities together. If they need to co-exist in the same file, they should be grouped logically in different regions of the file, so the reader can easily find methods right next to the declaration of the struct or enum they belong to.

sv2_frame::get_header

note from @Fi3:

Note for the future, it could make sense to return a type that encode the above infos: enum Ok, Bigger, Smaller. This would be an API change so for now I would live it like it is

error handling and edge cases

  • strict discipline over error handling (no unwrap, expect)
  • strict discipline over edge cases (no unimplemented!, todo!, panic!, unreachable!, etc)
@average-gary
Copy link
Contributor

code organization

Is this enforceable somehow?
If this is desirable, the CONTRIBUTING.md should be updated as well. (#1030)

@Fi3
Copy link
Collaborator

Fi3 commented Jul 4, 2024

I would allow unreachable and expect

@Fi3
Copy link
Collaborator

Fi3 commented Jul 4, 2024

A not on the payload panic. The API can and must be refactored in a way that avoid it just return a Result. When that is not possible (for reason x) IMO is better to panic and fail fast, rather the let the user use something in a wrong way.

@GitGab19
Copy link
Collaborator

New issue opened by @Fi3 could be related to this: #1147

@plebhash plebhash removed this from the 1.1.0 milestone Sep 3, 2024
@plebhash
Copy link
Collaborator Author

plebhash commented Sep 18, 2024

as discussed here #858 (comment)

the usage of generic B and the type alias Slice is a bit confusing... both are aiming to achieve very similar goals but in a redundant way

  • Sv2Frame<T, B> has generic B to allow the user to decide which kind of buffer they want (which could be anything, as long as it satisfies the trait contraints).
  • HandShakeFrame uses the Slice alias to choose between Vec<u8> vs buffer_sv2::Slice, and this choice is enacted via the with_buffer_pool

we should replace B with Slice everywhere and restrict user choices around Buffer kinds to only with_buffer_pool feature flag

as a result, both enum Frame<T> and struct Sv2Frame<T> should be generic only over T

@rrybarczyk
Copy link
Collaborator

In framing.rs, unit tests are handing without a mod tests block.

@jbesraa
Copy link
Contributor

jbesraa commented Oct 18, 2024

In framing.rs, unit tests are handing without a mod tests block.

This was addressed here 05f9b4c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Not necessary, but worth tracking refactor Implies refactoring code
Projects
Status: Ready For Review 🔍
7 participants