feat: essential field for packages #127
Conversation
Syntactic sugar for adding essentials to all slices defined in the package without having to go one by one.
rebornplusplus
left a comment
rebornplusplus
left a comment
There was a problem hiding this comment.
Cute PR. Looks good to me, thanks.
|
Nice.Thanks. This should be followed by https://warthogs.atlassian.net/browse/ROCKS-1038 |
cjdcordeiro
added
Simple
niemeyer
left a comment
niemeyer
left a comment
There was a problem hiding this comment.
Thanks, Alberto. A few comments:
internal/setup/setup_test.go
Outdated
| Package: "mypkg", | ||
| Name: "essential", | ||
| Essential: []setup.SliceKey{ | ||
| {"mypkg", "essential"}, |
There was a problem hiding this comment.
This one is awkward. We probably don't want self-references like this? This should probably be an error if explicit, and if implicit via the package-level essentials, we should drop it from this particular slice.
There was a problem hiding this comment.
Changed, it is now an error if a user adds the slice to the slice-level essentials. For package-level essentials, we just dont add the slice to itself.
niemeyer
left a comment
niemeyer
left a comment
There was a problem hiding this comment.
Thanks, Alberto. Probably the final pass.
internal/setup/setup.go
Outdated
| Package string | ||
| Name string | ||
| Essential []SliceKey | ||
| Essential map[SliceKey]bool |
There was a problem hiding this comment.
This has changed since the last review, and it doesn't look natural. The de-duplication is a simple local need inside the particular algorithm that is parsing the slices. It doesn't justify changing how the whole code sees the list of essentials.
There was a problem hiding this comment.
I have changed it back. Instead of creating a map I am iterating over the list of essentials now. I assume the list will not be long enough to justify hashmap vs iteration over a slice, and the map requires extra allocations.
niemeyer
left a comment
niemeyer
left a comment
There was a problem hiding this comment.
Looks good. Just a couple of trivials.
4 participants
Syntactic sugar for adding essentials to all slices defined in the package without having to go one by one.