-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor gera.Map + fix KV override with empty value issue + unit tests #603
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stupid question: what does "gera" stand for?
Enabled string `yaml:"enabled,omitempty"` | ||
} | ||
|
||
func kvStoreUnmarshalYAMLWithTags(w gera.Map[string, string], unmarshal func(interface{}) error) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this function be also tested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can indeed test it, but a test probably belongs in the workflow
package test and not in the one for the gera
package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it is worth it to write tests for this, as there is some logic there...
"dario.cat/mergo" | ||
) | ||
|
||
type Map interface { | ||
Wrap(m Map) Map | ||
type Map[K comparable, V any] interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason why use type arguments? I see only Map[string,string] used everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would probably be safe to make the key a string forever, not so sure about the value. Basically Map
already existed and I didn't want to preclude using non-string values in the future, if we so wish, mostly for the purposes of the template system, as well as using gera
in contexts other than the template system. Since Go got generics a few years ago, the untemplated gera.Map
became old style go, relying on interface{}
, so the paths forward were either (1) to remove it and only use gera.StringMap
, or (2) to update gera.Map
for modern Go. I choose 2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, thank you
"gopkg.in/yaml.v3" | ||
) | ||
|
||
var _ = Describe("hierarchical key-value store", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there some code coverage report generated? It would be nice to see whether you really test everything, because I cannot tell if everything is tested, from simple read through
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ginkgo -r -v -race --trace --coverpkg=github.com/AliceO2Group/Control/common/gera --coverprofile=.coverage-report.out
...
Ran 21 of 21 Specs in 0.023 seconds
SUCCESS! -- 21 Passed | 0 Failed | 0 Pending | 0 Skipped
PASS
coverage: 80.2% of statements in github.com/AliceO2Group/Control/common/gera
composite coverage: 80.2% of statements
Then go tool cover -html=.coverage-report.out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or actually, just make coverage
. AFAICT you added the command this April, and thanks for that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh heck... you are right, I already forgot about it 😆 I took a look at the coverage and it looks fine. However, I did not see any error path tests. But I trust you, that it is not necessary.
Not a stupid question at all. Back in the day I wanted a short package name (still do), which would have been |
Thanks, that explains it. Please add a word about it somewhere, e.g. at the top of |
Enabled string `yaml:"enabled,omitempty"` | ||
} | ||
|
||
func kvStoreUnmarshalYAMLWithTags(w gera.Map[string, string], unmarshal func(interface{}) error) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it is worth it to write tests for this, as there is some logic there...
https://its.cern.ch/jira/browse/OCTRL-916