Remove leading "/" from PathToStrings #122
Conversation
The "/" is only needed in formatted string from PathToString. Also add a short sanity unit test for PathToStrings, more detailed cases are already covered via TestPathToString.
ygot/pathstrings.go
Outdated
| @@ -43,7 +43,7 @@ const ( | |||
| // representing the path. | |||
There was a problem hiding this comment.
Suggest that we add a function comment here that says that the supplied path is always parsed as absolute and therefore should be the concatenation of the prefix plus path if a prefix exists.
There was a problem hiding this comment.
Clarified that path is absolute.
prefix is a property of Notification, and is not something this package deals with directly.
| @@ -109,6 +110,22 @@ func TestPathToString(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestPathToStrings(t *testing.T) { | |||
| in := &gnmipb.Path{Elem: []*gnmipb.PathElem{ | |||
There was a problem hiding this comment.
Can you make this the standard tests := []struct{ ... }; for _, tt := range tests { ... } layout please? It just makes things easier to extend when/if there are issues.
There was a problem hiding this comment.
I usually insist on the opposite in reviews: only add extra abstraction when it's actually needed.
Given the code sharing between PathToStrings and PathToString, more extra cases can be to TestPathToString.
|
ping @robshakir |
|
Sorry for the delay, LGTM. |
The "/" is only needed in formatted string from PathToString.
Also add a short sanity unit test for PathToStrings, more detailed cases
are already covered via TestPathToString.