Fix typo in packets-per-meaurement-period leaf - #1537
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![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 corrects a typo in the leaf name packets-per-meaurement-period to packets-per-measurement-period in the openconfig-oam-cfm.yang model. The reviewer suggested renaming the leaf to packets-per-measurement-interval instead, to maintain terminology consistency with the rest of the module which uses 'measurement interval' rather than 'measurement period'.
| leaf packets-per-measurement-period { | ||
| type uint16; | ||
| description "Number of probe packets to send within a measurement interval."; | ||
| } |
There was a problem hiding this comment.
There is a terminology inconsistency between the leaf name packets-per-measurement-period and the rest of the module. The module consistently uses the term "measurement interval" (e.g., the measurement-interval leaf on line 1206, and "measurement interval" in the description on line 1254).
To maintain consistency across the model, consider renaming this leaf to packets-per-measurement-interval.
Note: If you apply this change, please also update the reference in the description of packet-per-burst on line 1267. Additionally, since renaming a leaf is a backward-incompatible (breaking) change, the openconfig-version should be updated and a new revision entry should be added at the top of the file.
| leaf packets-per-measurement-period { | |
| type uint16; | |
| description "Number of probe packets to send within a measurement interval."; | |
| } | |
| leaf packets-per-measurement-interval { | |
| type uint16; | |
| description "Number of probe packets to send within a measurement interval."; | |
| } |
|
renaming the leaf is a backward incompatible change in this case. Not sure if worth deprecation and adding a new leaf |
agreed - one would also think it's caught by the first implementor vs. blindly implemented w/ a typo as well. No harm in introducing a new leaf, marking the existing as |
|
|
||
| oc-ext:openconfig-version "0.1.0"; | ||
|
|
||
| revision "2024-09-11" { |
There was a problem hiding this comment.
Add a new revision + increment version/reference per comments on compatibility
|
|
||
| leaf packets-per-meaurement-period { | ||
| leaf packets-per-measurement-period { | ||
| type uint16; |
There was a problem hiding this comment.
Whether defining a new leaf or modifying existing, I'd probably suggest filling in any missing units statements that should otherwise be there.
3 participants
Fix typo in packets-per-meaurement-period leaf