link: Change LinkInfoData space placements#29
Conversation
|
@cathay4t Let me know if you think this change is required, or we can leave it as it is now |
This is done to set a standard way for using space when implementing the Display trait. Putting the spaces the other way around (after each field), can cause a double space to appear, which might not be caught by tests (like happened with vxlan), because it depends on optional fields.
yonatan-linik
force-pushed
the
change_ifaces_space_placement
branch
from
c068844 to
e610737
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request standardizes the space placement in Display implementations for CliLinkInfoDataBond and CliLinkInfoDataBridge by moving spaces from the end of format strings to the beginning of subsequent fields, preventing double spaces when optional fields are omitted. Reviewers suggested applying this same pattern to CliLinkInfoDataBondPort and CliLinkInfoDataBridgePort for consistency. Additionally, there is a recommendation to use the captured variable shorthand {v} in several write! calls within CliLinkInfoDataBridge.
| write!(f, "broadcast_neighbor {} ", on_off(self.broadcast_neighbor))?; | ||
| write!(f, "ad_select {} ", self.ad_select)?; | ||
| write!(f, "tlb_dynamic_lb {}", self.tlb_dynamic_lb)?; | ||
| write!(f, "mode {}", self.mode)?; |
There was a problem hiding this comment.
The PR aims to standardize space placement in Display implementations to avoid double spaces with optional fields. However, CliLinkInfoDataBondPort::fmt (lines 257-262) still uses the old style with trailing spaces. For consistency and to ensure the standard is applied across all link info data structures, it should be updated as well.
| write!(f, "priority {} ", self.priority)?; | ||
| write!(f, "vlan_filtering {} ", self.vlan_filtering)?; | ||
| write!(f, "vlan_protocol {} ", self.vlan_protocol)?; | ||
| write!(f, "forward_delay {}", self.forward_delay)?; |
There was a problem hiding this comment.
Similar to the bond interface, the Display implementation for CliLinkInfoDataBridgePort (lines 642-696) has not been updated to the new space placement standard. It should be updated to maintain consistency and prevent the double-space issues described in the PR description.
| write!(f, " gc_timer {}", format_bridge_timer(self.gc_timer))?; | ||
| if let Some(v) = self.fdb_n_learned { | ||
| write!(f, "fdb_n_learned {} ", v)?; | ||
| write!(f, " fdb_n_learned {}", v)?; |
There was a problem hiding this comment.
For consistency with other parts of this function (e.g., line 383), consider using the captured variable shorthand {v} in the format string. This improvement can be applied to other similar write! calls in this implementation (lines 362, 367, 370, 431, 434, and 437).
| write!(f, " fdb_n_learned {}", v)?; | |
| write!(f, " fdb_n_learned {v}")?; |
|
As long as CI is happy. I am OK with this kind of fix. |
|
This project is designed to make |
2 participants
This is done to set a standard way for using space when implementing the Display trait.
Putting the spaces the other way around (after each field), can cause a double space to appear, which might not be caught by tests (like happened with vxlan), because it depends on optional fields.