Move version APIs to the specs-go package #214
Conversation
bart0sh
force-pushed
the
PR017-move-version-to-go-specs-go
branch
from
f3ac289
to
591c2a0
Compare
Signed-off-by: Ed Bartosh <[email protected]>
Signed-off-by: Ed Bartosh <[email protected]>
bart0sh
force-pushed
the
PR017-move-version-to-go-specs-go
branch
from
591c2a0
to
1e39659
Compare
|
@elezar can you review this PR? |
bart0sh
force-pushed
the
PR017-move-version-to-go-specs-go
branch
3 times, most recently
from
1ff42ed
to
93c6586
Compare
| @@ -525,7 +525,7 @@ func specType(content []byte) string { | |||
| } | |||
|
|
|||
| func TestCurrentVersionIsValid(t *testing.T) { | |||
| require.NoError(t, validateVersion(cdi.CurrentVersion)) | |||
| require.NoError(t, cdi.ValidateVersion(&cdi.Spec{Version: cdi.CurrentVersion})) | |||
| } | |||
|
|
|||
| func TestRequiredVersion(t *testing.T) { | |||
There was a problem hiding this comment.
We could consider moving these to specs-go/version_test.go.
There was a problem hiding this comment.
This would add a bunch of dependencies to specs-go/go.mod/sum, which would make it harder to import into k/k
There was a problem hiding this comment.
That is true. Let's leave it where it is then. does it at least make sense to move them to a version_text.go file in this package then with note on their intent?
Would implementing them in a specs_test package improve the dependency situation?
Signed-off-by: Ed Bartosh <[email protected]> Co-authored-by: Evan Lezar <[email protected]>
Signed-off-by: Ed Bartosh <[email protected]>
Signed-off-by: Ed Bartosh <[email protected]> Co-authored-by: Evan Lezar <[email protected]>
bart0sh
force-pushed
the
PR017-move-version-to-go-specs-go
branch
from
30d0688
to
54e62a8
Compare
specs-go/parser.go
Outdated
|
|
||
| import "strings" | ||
|
|
||
| // ParseQualifier splits a device qualifier into vendor and class. |
There was a problem hiding this comment.
It's out of scope for this PR, but we may want to be stricter on our language here. In the spec the entity we're parsing here is the Kind, but this public function refers to it as a qualifier. Since we're in the specs package here, do we want to rename this to SplitKind(string) instead?
There was a problem hiding this comment.
Generally it makes sense to me. I'd propose to rename it to ParseKind if you don't mind. And I agree, it's out of scope of this PR.
There was a problem hiding this comment.
OK. Let's do this as a follow-up then. (ParseKind) is fine.
Signed-off-by: Ed Bartosh <[email protected]>
Signed-off-by: Ed Bartosh <[email protected]>
bart0sh
force-pushed
the
PR017-move-version-to-go-specs-go
branch
from
b1e626c
to
7dd001f
Compare
This is a partial fix for #206