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

Restructure IbusMsg #33

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Restructure IbusMsg #33

wants to merge 2 commits into from

Conversation

Paul-weqe
Copy link
Member

IbusMsg had too many items (which can only keep growing). This commit restructures
the actions and categorize each item e.g Interface/RouteId and actions like add, delete,
update etc...

IbusMsg had too many items (which can only
keep growing). This commit restructures
the actions and categorize each item e.g
Interface/RouteId and actions like add,
delete, update etc...

Signed-off-by: Paul Wekesa <[email protected]>
@Paul-weqe
Copy link
Member Author

@rwestphal tests failing with an issue from the BGP tests. Can you help with that ?

Copy link
Member

@rwestphal rwestphal left a comment

Choose a reason for hiding this comment

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

Love this!

One thing we could do is derive EnumAsInner for the IbusMsg enum, which would let us leverage the From trait like this:

Instead of:

    let msg = IbusMsg::RouteMpls(RouteMplsMsg::Add(msg));
    let _ = ibus_tx.send(msg);

We could simplify it to:

    let msg = RouteMplsMsg::Add(msg);
    let _ = ibus_tx.send(msg.into());

@rwestphal tests failing with an issue from the BGP tests. Can you help with that ?

Our data-driven test framework uses JSON data to represent event messages, including ibus messages. By changing the IbusMsg enum, we need to regenerate all the test topologies data (which can be done automatically with a single command). Some conformance tests will need to be updated manually though, but only for input events. Since I'll be traveling in a couple of days, let's hold off on this for now.

@Paul-weqe
Copy link
Member Author

Paul-weqe commented Oct 23, 2024

@rwestphal are you talking about manually implementing the From trait on the enums ?

Looking through the enum-as-inner docs and testing locally. Finding it difficult to see a way we can run let _ = ibus_tx.send(msg.into()); without implementing the trait ourselves.

@rwestphal
Copy link
Member

@Paul-weqe You're absolutely right. The enum-as-inner crate only handles the conversion from the enum type to its variants, but not the other way around. But we can possibly use the derive-more crate to avoid implementing those conversions manually. The less code we need to write, the better.

Add derive_more crate.
Simplifies the calling of the
IbusMsg messages.

Signed-off-by: Paul Wekesa <[email protected]>
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.

2 participants