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.

Sign up for GitHub

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

Jump to bottom

Please remove assert from library code #16

Open
cathay4t opened this issue Apr 14, 2023 · 2 comments
Open

Please remove assert from library code #16

cathay4t opened this issue Apr 14, 2023 · 2 comments

Comments

@cathay4t
Copy link
Member

The traffic_control folder has many assert, please replace them with fn XXX() -> Result<(), Error::InvalidNla>
sophacles pushed a commit to sophacles/rtnetlink that referenced this issue Apr 14, 2023
Remove asserts that block re-assignment. Re: rust-netlink#16
25f0353 
The asserts removed in this commit all follow the pattern:

assert_eq!(some_struct.member, DEFAULT_VAL);

These asserts seem to only exist for preventing re-assignment of struct
values if they have already been assigned. This doesn't match what the
builder pattern usually does in rust. Rather than implementing an error
for this case,  and giving these methods the signature:

fn setter(self, val) -> Resutl<Self, Error::ReAssignment>

I removed the asserts to retain the standard builder pattern signature:

fn setter(self, val) -> Self

Signed-off-by: Erich Heine <[email protected]>
sophacles pushed a commit to sophacles/rtnetlink that referenced this issue Apr 14, 2023
Replace assert with Result. Re: rust-netlink#16
e9bca14 
Rather than panicking the calling application when the message Kind has
already been set, return Err(Error::InvalidNla).

Note: This adds the error type: Error::InvalidNla

Signed-off-by: Erich Heine <[email protected]>
@wllenyj
Copy link
Contributor

wllenyj commented Apr 19, 2023

The use of assert was to prevent the user from making mistakes and using the wrong API, such as calling ingress().egress(), ingress and egress are different Requests, Serial calls to ingress().egress() are wrong usage.

If there is a better way to achieve it is welcome.

cathay4t pushed a commit that referenced this issue Jul 10, 2023
@cathay4t
Remove asserts that block re-assignment. Re: #16
e6bcf3e 
The asserts removed in this commit all follow the pattern:

assert_eq!(some_struct.member, DEFAULT_VAL);

These asserts seem to only exist for preventing re-assignment of struct
values if they have already been assigned. This doesn't match what the
builder pattern usually does in rust. Rather than implementing an error
for this case,  and giving these methods the signature:

fn setter(self, val) -> Resutl<Self, Error::ReAssignment>

I removed the asserts to retain the standard builder pattern signature:

fn setter(self, val) -> Self

Signed-off-by: Erich Heine <[email protected]>
cathay4t pushed a commit that referenced this issue Jul 10, 2023
@cathay4t
Replace assert with Result. Re: #16
b7f8c73 
Rather than panicking the calling application when the message Kind has
already been set, return Err(Error::InvalidNla).

Note: This adds the error type: Error::InvalidNla

Signed-off-by: Erich Heine <[email protected]>
@cathay4t
Copy link
Member Author

Since this is crate/library, panic is never acceptable, we should never expect our user to catch panic. The Result<> is the only way a library/crate should used for failures.

If you want to assert user's input, please do that in the program using rtnetlink, not within, we are not CLI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@cathay4t @wllenyj