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

[Merged by Bors] - refactor: remove unnecessary bounds for encoder and decoder derive macro #3030

Closed
wants to merge 21 commits into from

Conversation

sehz
Copy link
Contributor

@sehz sehz commented Mar 1, 2023

Fixed a long standing tech debt around Encoder and Decoder derived macro.
With this fix, no longer need to add bounds to struct as before:

pub struct Message<C>
 where
     C: Encoder + Decoder + Debug,
{
     pub header: MsgType,
     pub content: C,
 }

Instead, this can be simplified:

pub struct Message<C> {
     pub header: MsgType,
     pub content: C,
 }

Forcing to add structure bounds cause downstream code to deal with extras bounds which make certain implementation difficult if not outright implementation

In addition, tracing code is only added if enabled. This should only necessary in development. This will reduce some code and improve performance slightly.

#[derive(Encoder,Decoder,Default]
#[fluvio(trace)]
MyStruct {
...
}

@sehz sehz marked this pull request as draft March 1, 2023 00:37
@sehz sehz added this to the 0.10.6 milestone Mar 1, 2023
@sehz sehz changed the title refactor: protocol min refactor: remove unnecessary bounds for encoder and decoder derive macro Mar 1, 2023
@sehz sehz marked this pull request as ready for review March 1, 2023 02:39
@sehz
Copy link
Contributor Author

sehz commented Mar 2, 2023

bors r+

bors bot pushed a commit that referenced this pull request Mar 2, 2023
…cro (#3030)

Fixed a long standing tech debt around Encoder and Decoder derived macro.
With this fix, no longer need to add bounds to struct as before:
```
pub struct Message<C>
 where
     C: Encoder + Decoder + Debug,
{
     pub header: MsgType,
     pub content: C,
 }
```

Instead, this can be simplified:
```
pub struct Message<C> {
     pub header: MsgType,
     pub content: C,
 }
```

Forcing to add structure bounds cause downstream code to deal with extras bounds which make certain implementation difficult if not outright implementation

In addition, tracing code is only added if enabled.  This should only necessary in development.  This will reduce some code and improve performance slightly.
```
#[derive(Encoder,Decoder,Default]
#[fluvio(trace)]
MyStruct {
...
}
```
@morenol
Copy link
Contributor

morenol commented Mar 2, 2023

not sure if we should merge this yet, since not sure if we need to do fluvio release again 0.10.5

@sehz
Copy link
Contributor Author

sehz commented Mar 2, 2023

reverting release should be only done in rare circumstances. next time, we should do another release.

@bors
Copy link

bors bot commented Mar 2, 2023

Pull request successfully merged into master.

Build succeeded:

  • Done

@bors bors bot changed the title refactor: remove unnecessary bounds for encoder and decoder derive macro [Merged by Bors] - refactor: remove unnecessary bounds for encoder and decoder derive macro Mar 2, 2023
@bors bors bot closed this Mar 2, 2023
@sehz sehz deleted the refactor_protocol_min branch March 11, 2023 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants