#132 add support for terminology valueset compose attribute #133
Conversation
evan-gordon
left a comment
evan-gordon
left a comment
There was a problem hiding this comment.
Overall really good stuff here! I left a few comments, mostly styling and cleanup. I'm happy to approve once the comments are addressed :)
interpreter/expressions.go
Outdated
| return false, fmt.Errorf("internal error - expected a ValueSetValue instead got %v", reflect.ValueOf(vs.GolangValue()).Type()) | ||
| } | ||
| // Handle different types of codes expressions | ||
| switch codesGolang := codesValue.GolangValue().(type) { |
There was a problem hiding this comment.
nit: IMO codesGolang isn't a good variable name, consider switching to cv here or something similar.
terminology/expansion_test.go
Outdated
| }) | ||
| } | ||
|
|
||
| func TestExpandValueSetEdgeCases(t *testing.T) { |
There was a problem hiding this comment.
Typically we stick to the golang table driven tests standard: https://go.dev/wiki/TableDrivenTests
for this case we could have something like:
tests := []struct {
name: string
valueSet: string
wantErrContains: string
}{
...
}
would you mind making that change for this and other tests in this file?
terminology/expansion_test.go
Outdated
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package terminology |
There was a problem hiding this comment.
This should be terminology_test similar to how it is interminology/local_test.go
| @@ -0,0 +1,480 @@ | |||
| // Copyright 2024 Google LLC | |||
There was a problem hiding this comment.
this looks like a duplicate of terminology/expansion_test.go please remove.
There was a problem hiding this comment.
Why were both terminology_expansion_test.go and expansion_test.go deleted? It looks like you mightve renamed it to terminology_test.go? I only asked that the package name get changed, not the file name as well :)
There was a problem hiding this comment.
I thought you were asking me to makea a terminology_test like local_test.go? I was using this comment: #133 (comment)
There was a problem hiding this comment.
Sorry for the confusion that's my mistake. I was specifically referring to the package name. The file name was fine :)
I'll try to be more explicit in the future for cases like this.
terminology/terminology_test.go
Outdated
| t.Run(tt.name, func(t *testing.T) { | ||
| codes, err := provider.ExpandValueSet(tt.valueSetURL, tt.version) | ||
|
|
||
| if tt.expectError { |
There was a problem hiding this comment.
For all of these tests could you move the error case to it's own test func? I've found that test logic is a lot more readable and easy to maintaine when we do a different test grouping for error cases.
2 participants
No description provided.