LldpAdminStatus: Remove Display and FromStr impls#10010
Merged
jgallagher merged 3 commits intomainfrom Mar 10, 2026
Merged
Conversation
jgallagher
commented
Mar 9, 2026
| LldpAdminStatus::Enabled => "enabled", | ||
| LldpAdminStatus::Disabled => "disabled", | ||
| LldpAdminStatus::RxOnly => "rx only", | ||
| LldpAdminStatus::TxOnly => "tx only", |
Contributor
Author
There was a problem hiding this comment.
Bikeshedding welcome on these. Capitalize TX/RX? Get rid of the space? This is displayed in a bulleted list, so I don't think it really needs to match what's in the config file, but if folks disagree we could make this go through serde serialization to match.
smklein
approved these changes
Mar 9, 2026
Collaborator
smklein
left a comment
smklein
left a comment
There was a problem hiding this comment.
lgtm, I refuse to be baited into bikeshedding! I think your decisions are fine!
Contributor
This justification seems sound to me! |
internet-diglett
approved these changes
Mar 10, 2026
jgallagher
added a commit
that referenced
this pull request
Mar 10, 2026
Followup to #10010, and pretty similar: * There was only one consumer of `FromStr`; it was a test and was not using the majority of the parsing complexity (which handled both the special string `"link-local"` and optional VLANs, neither of which the test used). The test now goes through a new constructor (which may itself be overkill; I'll revisit it as a part of #9832). * There was only two consumers of `Display` - `sled-agent` and `wicketd`, both setting SMF properties - and those now use an explicitly-named method so we don't assume an `UplinkAddressConfig` can generally be converted to a `String`.
jgallagher
added a commit
that referenced
this pull request
Mar 12, 2026
Reasoning here is similar to #10010: this isn't really a naturally-displayable type, and in this case we were previously abusing this to stringify it both in the db (fixed by #9984) and the external API (fixed by #10014, on which this PR is based). Maybe worth noting: I expected this to be entirely futureproofing, but by doing this I found a few external API types that I'd missed in an earlier draft of #10014 (types where we stringified `SwitchSlot`s for _output_ but never parsed them as _input_). All our remaining uses of the display impl are either for logging or constructing internal error messages; I changed all of these to use `Debug`, and added a manual `Debug` implementation that matches the old `Display` implementation. This should keep the error messages and logs consistent while not leading us to the pit of sadness of assuming we can format and parse these values as strings without consideration.
This was referenced Mar 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is another bit on the yak stack of #9832. A handful of these low-level networking types implement
DisplayandFromStr, and I don't think they should. They don't have a natural representation the way a string, number, or IP address does. This PR removes those impls fromLldpAdminStatusand I think is a good example of this. There were no consumers of theFromStrimplementation, and there were 3 consumers of theDisplayimpl, but really they want 3 different things:lldpd(the most reasonable use ofDisplay, but I think it's clearer to make this an explicitly named method)match: there's no reason we have to reuse the same representationlldpdwants, and arguably we can make it look nicer by at least putting spaces in the"tx only"/"rx only"variants - feedback welcome here)config-rss.toml- this use was actually incorrect, and should have been going through serde serialization for the correct value. This didn't cause any problems in practice because ourDisplayimpl happened to use the same representation as serde, but this is the kind of happy coincidence that can easily break.