Improve API robustness and remove public dependencies #21
Conversation
The #[from] attribute of thiserror also implements the Error::source() method which allows better error reporting compared to a custom From implementation.
With this re-export users of snmp-parser no longer need to also depend on asn1-rs. This prevents some nasty probelms when the asn1-rs version used in this crate and the version used by the user differ, making the crate's API more robust overall.
Using nom::internal::IResult in the crate's public API required users to depend on nom as well for error handling. This dependency is removed by returning the std Result in the public API. For now the 'nom parser functions' are preserved as the internal_ functions so we can make them public again if required.
|
@chifflier Any opinions on this PR? |
|
Hi, |
There was a problem hiding this comment.
There are some fixes and some ideas that I'd like to take separately. With your permission, I'll cherry-pick them.
The only problematic part in my opinion, is the API change. I'll leave this part for now, and since I'll work on adding the FromBer type everywhere, maybe this can be re-thought again after.
| pub fn parse_snmp_generic_message(i: &[u8]) -> Result<(&[u8], SnmpGenericMessage), SnmpError> { | ||
| internal_parse_snmp_generic_message(i).finish() | ||
| } |
There was a problem hiding this comment.
I understand why merging error types with .finish() may seem interesting, but the problem is that it prevents chaining the parser combinators (for ex, you won't be able to use many1(parse_snmp_generic_message).
Another problem is that it breaks backwards API.
Since the new API in asn1-rs is more focused around FromBer, this is maybe an intermediate solution: providing helper functions (or just let caller add .finish() ...) with different names (to not break API), in addition to the default ones
| @@ -48,3 +48,6 @@ pub mod snmpv3; | |||
| pub use generic::*; | |||
| pub use snmp::*; | |||
| pub use snmpv3::*; | |||
|
|
|||
| // re-exports to prevent public dependency on asn1_rs | |||
| pub use asn1_rs::{Oid, OidParseError}; | |||
There was a problem hiding this comment.
My other crates also re-export types, I believe it is right to reexport required types (maybe from nom too)
|
As commented on the PR, I'll start by cherry-picking some patches. |
|
Ok, 3 commits have been cherry-picked. As explained, closing the PR, please open a new one for anything new or to follow discussions |
Hi,
thanks for implementing this parser crate. It helps a lot. However, while including the crate in my business logic a found some unergonomic API designs that this PR tries to solve.
The main problem I found is that there are types of the crate's dependencies in the crate's public API. Namely, types from asn1-rs and nom. This required me to also depend on these crate to handle OIDs and to handle nom parser errors. But this gets problematic when versions between snmp-parser, asn1-rs and nom start to differ. Actually, there is already a difference between nom v7.0 used in snmp-parser and the current nom version v7.1.
This PR removes those public dependencies via re-export or returning std types instead. Feel free to cherry pick single commits. I'm also open to discuss alternative solutions.