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

feat(scmp): add enums for error and info messages #141

Merged
merged 1 commit into from
Feb 7, 2024

Conversation

mlegner
Copy link
Contributor

@mlegner mlegner commented Feb 6, 2024

Closes #130

Copy link
Contributor

github-actions bot commented Feb 6, 2024

Code Coverage

Package Line Rate Health
crates/scion-proto/src/path/metadata 100%
crates/scion/src/daemon 91%
crates/scion-proto/src/packet 80%
crates/scion-proto/src/path 83%
crates/scion-proto/src/reliable 95%
crates/scion-proto/src/address 69%
crates/scion/src/pan/path_strategy 83%
crates/scion/src 80%
crates/scion-proto/src/packet/headers 85%
crates/scion/src/socket 77%
crates/test-utils/src 100%
crates/scion-proto/src/scmp 72%
crates/scion/src/pan 78%
crates/scion-proto/src/path/standard 92%
crates/scion-proto/src 78%
Summary 79% (1940 / 2470)

@mlegner mlegner marked this pull request as ready for review February 6, 2024 13:53
Copy link
Contributor

@jpcsmith jpcsmith left a comment

Choose a reason for hiding this comment

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

Hey Markus, looks good.

Suggestion: Move the methods from the Scmp(Informational|Error)MessageTrait traits to be onto the respective enums and variants, and remove the traits. A user can take either an Scmp(Informational|Error)Message, or one of it's variants to their function. Given the enum type, the trait feels unecessary.

crates/scion-proto/src/scmp/messages.rs Outdated Show resolved Hide resolved
@mlegner mlegner requested a review from jpcsmith February 6, 2024 18:33
@jpcsmith jpcsmith merged commit 07f05d7 into main Feb 7, 2024
11 checks passed
@jpcsmith jpcsmith deleted the feat/scmp-message-types branch February 7, 2024 16:43
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.

Add enums ScmpErrorMessage and ScmpInformationalMessage, leaving ScmpMessage unchanged
2 participants