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: Replace trait Decode with Decoder #337

Merged
merged 2 commits into from
Aug 8, 2023

Conversation

jakoschiko
Copy link
Collaborator

Replaces the current trait Decode with the trait Decoder in order to improve lifetime issues.

I don't expect this PR to be merged, I just want to demonstrate my ideas. So, what's the problem with the current trait Decode?

pub trait Decode<'a>: Sized {
    fn decode(input: &'a [u8]) -> Result<(&'a [u8], Self), DecodeError>;
}

If I use this trait with type bounds, then I need to choose the lifetime in the wrong place:

fn foo<'a, T: Decode<'a>>() -> T { // The caller chooses a lifetime 'a
    let vec: Vec<u8> = b"foo".to_vec();
    let slice: &[u8] = &vec; // The slice's lifetime does not match lifetime 'a
    T::decode(slice); // Does not compile
}

This prevents me from writing useful abstractions based on Decode. So, here my proposed trait Decoder.

pub trait Decoder {
    type Item<'a>: Sized;

    fn decode<'a>(input: &'a [u8]) -> Result<(&'a [u8], Self::Item<'a>), DecodeError>;
}

It allows me to choose the lifetime at a later point in time:

fn foo<T: Decoder>() {
    let vec: Vec<u8> = b"foo".to_vec();
    let slice: &[u8] = &vec;
    T::decode(slice); // The lifetime is chosen here, now it compiles
}

List of changes:

  • I replaced the trait Decode with Decoder
  • I introduced the structs CommandDecoder, ResponseDecoder, etc. that implement Decoder
  • I replaced the trait DecodeStatic with StaticDecoder that I was able to implement with less code duplication
  • I fixed all usages of Decode

If you like the changes, we could do even more:

  • For symmetry reasons we replace the trait Encode with Encoder
  • We replace CommandDecoder, ResponseDecoder, etc. with CommandCodec, ResponseCodec, etc. that implement both Decoder and Encoder

@coveralls
Copy link
Collaborator

coveralls commented Aug 5, 2023

Pull Request Test Coverage Report for Build 5797418516

  • 29 of 35 (82.86%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.06%) to 93.593%

Changes Missing Coverage Covered Lines Changed/Added Lines %
imap-codec/src/codec.rs 6 12 50.0%
Totals Coverage Status
Change from base Build 5796696502: -0.06%
Covered Lines: 8706
Relevant Lines: 9302

💛 - Coveralls

@duesee
Copy link
Owner

duesee commented Aug 7, 2023

This makes so much more sense, thanks! 👍 (As discussed offline, let's talk about how, and when, to merge this later.)

@jakoschiko jakoschiko changed the title reactor: Replace trait Decode with Decoder refactor: Replace trait Decode with Decoder Aug 7, 2023
@duesee
Copy link
Owner

duesee commented Aug 8, 2023

As discussed yesterday, I moved the *Decoder structs into codec and renamed them to *Codec. As you proposed, I also removed the StaticDecoder by making it a default trait impl. Is there anything left we want to do in this PR?

Two possible paths to continue: 1) Continue with Encoder or 2) #333.

@jakoschiko
Copy link
Collaborator Author

I suggest this order: merge this, merge #333, add Encoder

@duesee duesee enabled auto-merge (rebase) August 8, 2023 13:15
@duesee duesee merged commit b4872d9 into duesee:main Aug 8, 2023
10 checks passed
@jakoschiko jakoschiko deleted the jakoschiko/decoder branch May 29, 2024 19:59
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