Add reverse string lookups for IpNumber and NdpOptionType#146
Add reverse string lookups for IpNumber and NdpOptionType#146vishy11 wants to merge 1 commit intoJulianSchmid:masterfrom
Conversation
Add parsing helpers for the string values already exposed by the existing lookup APIs. - add IpNumber::from_keyword_str and IpNumber::from_protocol_str - add NdpOptionType::from_keyword_str - derive forward and reverse lookups from shared match tables so the mappings stay aligned - cover round-trip behavior and duplicate protocol-string handling with tests
📝 WalkthroughWalkthroughAdds bidirectional string conversion APIs for network protocol types: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
etherparse/src/net/ip_number_impl.rs (2)
709-711: Optional: keepfrom_*methods consistent on#[must_use].
Option-returning constructors are good candidates for#[must_use]so callers don't accidentally drop a parsed result. Minor stylistic suggestion only.♻️ Proposed change
+ #[must_use] pub fn from_keyword_str(keyword: &str) -> Option<Self> { ip_number_keywords!(keyword, number_by_str) } @@ + #[must_use] pub fn from_protocol_str(protocol: &str) -> Option<Self> { ip_number_protocols!(protocol, number_by_str) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@etherparse/src/net/ip_number_impl.rs` around lines 709 - 711, Add the #[must_use] attribute to the Option-returning constructor to match other from_* methods: annotate the function from_keyword_str(keyword: &str) -> Option<Self> with #[must_use] so callers are warned when they drop the parsed result; update the function signature for from_keyword_str accordingly (preserving the existing body that calls ip_number_keywords!(keyword, number_by_str)).
340-356: Macros: alias handling for reverse lookup is silently asymmetric — please confirm intent (and document).
str_by_number!honors aliases via$num $(| $alias)*, butnumber_by_str!only emits$text => Some(Self($num))and discards$alias. For the only current multi-number row (253 | 254 => "Use for experimentation and testing"), this is exactly the documented "first assigned number is returned" behavior on Line 732, so it's consistent with intent.Two follow-ups to consider:
- The doc on
from_keyword_str(Lines 697-708) does not mention the same first-wins rule even thoughkeyword_strcould grow aliased rows the same way later. Consider adding a parallel sentence so contributors don't add a new aliased row expecting reverse parsing of the alias text.- Note that adding two separate rows sharing the same
$text(without|) would produce duplicate match arms innumber_by_str!and trigger an "unreachable pattern" warning. That's actually a useful guard rail; you may want a brief comment near the macro definition documenting this constraint so future contributors use the|form intentionally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@etherparse/src/net/ip_number_impl.rs` around lines 340 - 356, The macros str_by_number! and number_by_str! are asymmetrical: str_by_number! supports alias arms via "$num | $alias" but number_by_str! ignores aliases and will emit duplicate match arms if two distinct rows use the same $text, which is intentional for the current 253|254 row but should be documented; update docs and code comments to make intent explicit by (1) adding a sentence to the from_keyword_str doc (the function mentioned in the review) that parsing uses the first-assigned number when multiple numeric aliases share the same keyword, and (2) add a brief comment above the macro definitions (str_by_number! and number_by_str!) stating that aliases must be declared using the "| alias" form to avoid duplicate text arms and that number_by_str! will pick the first matching numeric assignment; don't change macro behavior, just document the first-wins rule and the duplicate-arm guard so future contributors use the alias form intentionally.etherparse/src/transport/icmpv6/ndp_option_type_impl.rs (1)
33-57: Consider sharing a single mapping table betweenkeyword_strandfrom_keyword_str.The forward (Lines 34-43) and reverse (Lines 48-57) lookups duplicate the five keyword/number pairs. The companion change in
etherparse/src/net/ip_number_impl.rsdeliberately introducesstr_by_number!/number_by_str!macros with sharedip_number_keywords!/ip_number_protocols!tables for exactly this reason — to keep forward and reverse mappings aligned. The round-trip test at Lines 100-107 will catch divergence, but the duplication is still a maintenance hazard if a new option type is added in only one place.Optional: apply the same macro-based pattern here, or at minimum extract a single
matchinto a helper that both directions consume.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@etherparse/src/transport/icmpv6/ndp_option_type_impl.rs` around lines 33 - 57, The keyword_str and from_keyword_str implementations duplicate the same five mappings for NdpOptionType; refactor to centralize the mapping (e.g., a single static array/constant or a macro like ip_number_keywords!) and have both functions consult that single source: create a shared mapping (pairing numbers and &'static str) referenced by keyword_str (lookup by number) and from_keyword_str (lookup by string) or produce a macro that emits both match arms so additions stay in one place; update keyword_str, from_keyword_str and any tests to use the shared mapping and keep the symbol names NdpOptionType, keyword_str, and from_keyword_str for easy location.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@etherparse/src/net/ip_number_impl.rs`:
- Around line 709-711: Add the #[must_use] attribute to the Option-returning
constructor to match other from_* methods: annotate the function
from_keyword_str(keyword: &str) -> Option<Self> with #[must_use] so callers are
warned when they drop the parsed result; update the function signature for
from_keyword_str accordingly (preserving the existing body that calls
ip_number_keywords!(keyword, number_by_str)).
- Around line 340-356: The macros str_by_number! and number_by_str! are
asymmetrical: str_by_number! supports alias arms via "$num | $alias" but
number_by_str! ignores aliases and will emit duplicate match arms if two
distinct rows use the same $text, which is intentional for the current 253|254
row but should be documented; update docs and code comments to make intent
explicit by (1) adding a sentence to the from_keyword_str doc (the function
mentioned in the review) that parsing uses the first-assigned number when
multiple numeric aliases share the same keyword, and (2) add a brief comment
above the macro definitions (str_by_number! and number_by_str!) stating that
aliases must be declared using the "| alias" form to avoid duplicate text arms
and that number_by_str! will pick the first matching numeric assignment; don't
change macro behavior, just document the first-wins rule and the duplicate-arm
guard so future contributors use the alias form intentionally.
In `@etherparse/src/transport/icmpv6/ndp_option_type_impl.rs`:
- Around line 33-57: The keyword_str and from_keyword_str implementations
duplicate the same five mappings for NdpOptionType; refactor to centralize the
mapping (e.g., a single static array/constant or a macro like
ip_number_keywords!) and have both functions consult that single source: create
a shared mapping (pairing numbers and &'static str) referenced by keyword_str
(lookup by number) and from_keyword_str (lookup by string) or produce a macro
that emits both match arms so additions stay in one place; update keyword_str,
from_keyword_str and any tests to use the shared mapping and keep the symbol
names NdpOptionType, keyword_str, and from_keyword_str for easy location.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 34567b74-def2-42f5-ae81-45f8f0fb8440
📒 Files selected for processing (2)
etherparse/src/net/ip_number_impl.rsetherparse/src/transport/icmpv6/ndp_option_type_impl.rs
1 participant
Add reverse lookup helpers for string values already exposed by existing APIs.
IpNumber::from_keyword_stras the inverse ofIpNumber::keyword_strIpNumber::from_protocol_stras the inverse ofIpNumber::protocol_strNdpOptionType::from_keyword_stras the inverse ofNdpOptionType::keyword_strSummary by CodeRabbit
New Features
Tests