Initial modernization (WIP) #20
Conversation
…18 and latest dependencies. Errors now conforming to std::error::Error.
Cargo.toml
Outdated
| itertools = "0.10" | ||
| libc = "0.2" | ||
| nix = "0.23" | ||
| clap = "2.33" |
There was a problem hiding this comment.
I'd consider moving the bin/can.rs to examples/can.rs or creating a workspace with two separate crates to avoid the dependencies on anyhow and clap for projects that use the socketcan crate only for the library part.
There was a problem hiding this comment.
Yes, you're probably right. Normally I don't worry about it, but for a small, embedded crate like this the overhead is significant. I prefer to keep examples short and easy to understand, and keep what I consider "useful utilities" separate. A workspace seems a bit too complicated. Maybe just hide the utility(s) behind a feature flag?
Also I was debating the name. "can" seens a bit generic, though I want to keep it short and simple. "rcan"? "canrs"?
There was a problem hiding this comment.
I went with a feature flag.
|
Hi! Super excited to see this get worked on! |
src/lib.rs
Outdated
| impl error::Error for CanSocketOpenError { | ||
| fn description(&self) -> &str { | ||
| match *self { | ||
| CanSocketOpenError::LookupError(_) => "can device not found", | ||
| CanSocketOpenError::IOError(ref e) => e.description(), | ||
| } | ||
| } | ||
|
|
||
| fn cause(&self) -> Option<&error::Error> { | ||
| match *self { | ||
| CanSocketOpenError::LookupError(ref e) => Some(e), | ||
| CanSocketOpenError::IOError(ref e) => Some(e), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl error::Error for CanSocketOpenError {} |
There was a problem hiding this comment.
Since I can't comment on the enum itself since it's not in the diff... could it make more sense to move this error to the src/err.rs file since that seems to be the intended spot for most, if not all, error declarations?
There was a problem hiding this comment.
A few small comments since it seems like code reorganization is somewhat in scope given the changes so far.
Yes, I'm assuming that this will be a major update with lots of refactoring, and that breaking changes are OK as long as there's an overall consensus that we're moving in the right direction.
could it make more sense to move this error to the src/err.rs
Yes, I was thinking the same thing when I reviewed this work recently (the branch is from last year). I may have been thinking to just do a little at a time? But since this PR includes error refactoring as a goal, this is probably a good place to make the change.
Also... I typically prefer to export a single error type from a library, because it tends to make things simpler when you layer libraries on top of each other. And these days I just do it with thiserror. But I wasn't sure how people would feel about either of those things.
There was a problem hiding this comment.
Yeah, this is just pedantic/gut reaction nits, but it seems likely the error design might continue to change as you/whoever keeps working on refactoring/modernization.
End of the day, I guess this is more stream of consciousness than actual suggestion. :P
src/nl.rs
Outdated
| @@ -148,50 +45,84 @@ impl CanInterface { | |||
| /// Open CAN interface by name | |||
| /// | |||
| /// Similar to `open_if`, but looks up the device by name instead | |||
| pub fn open(ifname: &str) -> Result<CanInterface, nix::Error> { | |||
| pub fn open(ifname: &str) -> result::Result<Self, nix::Error> { | |||
There was a problem hiding this comment.
Why the result::Result? It's not competing with any other type being brought into scope.
There was a problem hiding this comment.
Did I do this? 😕 If so, it was likely because I was preparing for a crate-specific Error and associated Result, and didn't get to it. (yet?)
There was a problem hiding this comment.
That's what I get for commenting on a WIP PR. :P
There was a problem hiding this comment.
No, @tobz, you're right. I was going down that path, but received some feedback saying to keep it the way it is for now, and didn't clean up properly. I'm trying to find the balance of what people want to do in the next release or two.
src/lib.rs
Outdated
| } | ||
| } | ||
| } | ||
| impl error::Error for ConstructionError {} |
There was a problem hiding this comment.
Same comment as above about this error cohabiting with the others in src/err.rs.
|
@fpagliughi Just an FYI -- I am working on a J1939 SocketCan library and in the process preparing PRs to both It is a night/weekend project, so it will take a little more time to properly prepare. I will try to remember to comment back here when the PRs are up. |
|
Looking forward to getting this merged. If something should be done to make it possible - I can help. |
|
Would be great to see this work continue. I have a fairly pressing need for this, so I might end up forking in the meantime. Other things that would be great to add would be 1) CAN-FD support and 2) async support (simple tokio wrapper would do, |
|
FYI regarding the async support there is tokio-socketcan . Since this relies on the socketcan crate but progress is stuck I guess a fork is a good idea for CAN-FD. I think @nnarain also worked on a fork, so might make sense to look at that. |
|
Apologies for the slow start on this as I've had write access for a few months now. It got queued up in line behind a few other open-source projects. But I promise I'll get started within the next few weeks when I get back to a whole bunch of CAN work. |
This is an initial update to start work on this package again; to update the dependencies and start moving toward a more recent version of Rust.
neliv0.6nixdependency to latest v0.23