simplify FillDefault() using reflects
#1069
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AkihiroSuda
force-pushed
the
reflect
branch
from
c54861d
to
29d65ad
Compare
AkihiroSuda
force-pushed
the
reflect
branch
from
29d65ad
to
01a8a36
Compare
AkihiroSuda
changed the title
pkg/limayaml: remove deprecated network and useHostResolver ; simplify FillDefault()` using reflectspkg/limayaml: remove deprecated network and useHostResolver ; simplify FillDefault() using reflects
- `network`: deprecated in Lima v0.7.0 (Oct 2021), in favor of `networks`: lima-vm@07e6823 - `useHostResolver`: deprecated in Lima v0.8.1 (Jan 2022), in favor of `hostResolver.enabled`: lima-vm@eaeee31 The deprecated support for VDE is not removed in this commit. Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
AkihiroSuda
force-pushed
the
reflect
branch
from
01a8a36
to
2b3f4ca
Compare
Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
AkihiroSuda
force-pushed
the
reflect
branch
from
2b3f4ca
to
7cbfb37
Compare
jandubois
reviewed
Oct 9, 2022
| var err error | ||
| x, err = Merge(x, v) | ||
| if err != nil { | ||
| return x, err |
There was a problem hiding this comment.
Don't return incompletely merged result in case of error:
Suggested change
| return x, err | |
| return nil, err |
jandubois
reviewed
Oct 9, 2022
Comment on lines
+149
to
+154
| func logX(t testing.TB, x interface{}, format string, args ...interface{}) { | ||
| // Print in JSON for human readability | ||
| j, err := json.Marshal(x) | ||
| assert.NilError(t, err) | ||
| t.Logf(format+": %s", append(args, string(j))...) | ||
| } |
There was a problem hiding this comment.
args is never used; format is really just a name:
Suggested change
| func logX(t testing.TB, x interface{}, format string, args ...interface{}) { | |
| // Print in JSON for human readability | |
| j, err := json.Marshal(x) | |
| assert.NilError(t, err) | |
| t.Logf(format+": %s", append(args, string(j))...) | |
| } | |
| func logX(t testing.TB, x interface{}, name string) { | |
| // Print in JSON for human readability | |
| j, err := json.Marshal(x) | |
| assert.NilError(t, err) | |
| t.Logf("%s: %s", name, string(j)) | |
| } |
Personally I would also swap the order of x and name, but it doesn't really matter.
jandubois
reviewed
Oct 9, 2022
| } | ||
| } | ||
| // *EXCEPTION*: Mounts are appended in d, y, o order, but "merged" when the Location matches a previous entry; | ||
| // the highest priority Writable setting wins. |
There was a problem hiding this comment.
Just below is this block:
mounts := make([]Mount, 0, len(d.Mounts)+len(y.Mounts)+len(o.Mounts))
location := make(map[string]int)
for _, mount := range append(append(d.Mounts, y.Mounts...), o.Mounts...) {
if i, ok := location[mount.Location]; ok {It should have been updated to no longer use d, y, and o, but just x.
mounts := make([]Mount, 0, len(x.Mounts))
location := make(map[string]int)
for _, mount := range x.Mounts {
mount.Location = filepath.Clean(mount.Location)
if i, ok := location[mount.Location]; ok {Update: I also added a call to filepath.Clean() to normalize paths before matching. Could be in a separate commit or even PR.
There was a problem hiding this comment.
Same thing again further down for networks.
jandubois
reviewed
Oct 9, 2022
Comment on lines
+222
to
+224
| networks := make([]Network, 0, len(d.Networks)+len(y.Networks)+len(o.Networks)) | ||
| iface := make(map[string]int) | ||
| for _, nw := range append(append(d.Networks, y.Networks...), o.Networks...) { |
There was a problem hiding this comment.
Suggested change
| networks := make([]Network, 0, len(d.Networks)+len(y.Networks)+len(o.Networks)) | |
| iface := make(map[string]int) | |
| for _, nw := range append(append(d.Networks, y.Networks...), o.Networks...) { | |
| networks := make([]Network, 0, len(x.Networks)) | |
| iface := make(map[string]int) | |
| for _, nw := range x.Networks { |
AkihiroSuda
changed the title
pkg/limayaml: remove deprecated network and useHostResolver ; simplify FillDefault() using reflectsFillDefault() using reflects
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The type of
CPUType map[Arch]stringis now changed tomap[Arch]*string, aspkg/reflectutil.Merge()is designed to distinguish "empty" from "nil".