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 all the repetitive code in roles-logic-sv2 parsers file into macros #67

Open
Tracked by #48 ...
Fi3 opened this issue Nov 17, 2021 · 8 comments
Open
Tracked by #48 ...
Labels
backlog Not necessary, but worth tracking good first issue Good for newcomers help wanted Extra attention is needed

Comments

@Fi3
Copy link
Collaborator

Fi3 commented Nov 17, 2021

No description provided.

@Fi3 Fi3 added this to the Refactor messages-sv2 milestone Nov 17, 2021
@Fi3 Fi3 moved this to Todo in Sv2 Project Plan 2022 Nov 17, 2021
@Fi3 Fi3 added the good first issue Good for newcomers label Mar 31, 2022
@rrybarczyk
Copy link
Collaborator

In roles-logic-sv2/src/parser.rs we have a lot of copied code for each type. This would be nice to do with macros.

@rrybarczyk rrybarczyk added nice to have help wanted Extra attention is needed labels Sep 16, 2022
@rrybarczyk rrybarczyk changed the title refactor all the repetitive code into macros refactor all the repetitive code in roles-logic-sv2 parsers file into macros Sep 16, 2022
@pavlenex pavlenex added backlog Not necessary, but worth tracking and removed nice to have labels Dec 5, 2022
@vjtch
Copy link

vjtch commented Nov 23, 2023

Hi @rrybarczyk, I would like to work on this issue.

@vjtch
Copy link

vjtch commented Apr 7, 2024

Hi @pavlenex, I've got some time and I'd like to contribute. I already looked at this issue back in November. Is it still relevant?

@rrybarczyk
Copy link
Collaborator

@vjtch fantastic!

I am not sure if it is still relevant. You can look at the file and see if you find any duplicate logic that can be modularized. You can also look at the history of this file and see if you find any commits that look like it is addressing this issue. If you don't see anything that looks like it has consolidate the logic, I would say it is still a valid issue. @Fi3 will be able to confirm better than I can though.

@vjtch
Copy link

vjtch commented Apr 20, 2024

@rrybarczyk there is still some duplicated logic.

For example this

impl<'a> TryFrom<(u8, &'a mut [u8])> for CommonMessages<'a> {
    type Error = Error;

    fn try_from(v: (u8, &'a mut [u8])) -> Result<Self, Self::Error> {
        let msg_type: CommonMessageTypes = v.0.try_into()?;
        match msg_type {
            CommonMessageTypes::SetupConnection => {
                let message: SetupConnection<'a> = from_bytes(v.1)?;
                Ok(CommonMessages::SetupConnection(message))
            }
            CommonMessageTypes::SetupConnectionSuccess => {
                let message: SetupConnectionSuccess = from_bytes(v.1)?;
                Ok(CommonMessages::SetupConnectionSuccess(message))
            }
            // ...
        }
    }
}

can be rewrited to

macro_rules! try_from_for_common_messages {
    ( [ $( $variant:ident $( < $lifetime:lifetime > )? ),+ ] ) => {
        impl<'a> TryFrom<(u8, &'a mut [u8])> for CommonMessages<'a> {
            type Error = Error;

            fn try_from(v: (u8, &'a mut [u8])) -> Result<Self, Self::Error> {
                let msg_type: CommonMessageTypes = v.0.try_into()?;
                match msg_type {
                    $(
                        CommonMessageTypes::$variant => {
                            let message: $variant $( <$lifetime> )? = from_bytes(v.1)?;
                            Ok(CommonMessages::$variant(message))
                        }
                    )+
                }
            }
        }
    };
}

try_from_for_common_messages!([SetupConnection<'a>, SetupConnectionSuccess, SetupConnectionError<'a>, ChannelEndpointChanged]);

In this example there are only 4 variants of enum but there are similiar impl blocks where are enums with more than 20 variants (e.g. MiningTypes) and code is duplicated for all of them.

@pavlenex
Copy link
Collaborator

cc @jbesraa perhaps this issue is now duplication of some of other issues/pr's, it seems so, if not we can cross reference it in a tracker since this seems to be refactoring work.

@rrybarczyk
Copy link
Collaborator

cc @jbesraa perhaps this issue is now duplication of some of other issues/pr's, it seems so, if not we can cross reference it in a tracker since this seems to be refactoring work.

@pavlenex can you list the issue or PRs that this may be a duplicate of?

Anytime an issue like this pops up, we need to be diligent about adding it to #845. I just edited #845's issue to include two segments: 1) Rust Docs (this is pretty filled out) and 2) Refactor Implementation (new subsection addition). I linked this issue under the Refactor Implementation subsection to kick it off.

@jbesraa
Copy link
Contributor

jbesraa commented Aug 19, 2024

We did not refactor any repetitive code in roles yet so this is still to do

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 good first issue Good for newcomers help wanted Extra attention is needed
Projects
No open projects
Status: Todo 📝
Development

No branches or pull requests

5 participants