Skip to content

Commit

Permalink
chore: better go tooling (#2027)
Browse files Browse the repository at this point in the history
Replace FillDefaults -> WithDefaults.

Let WithDefaults() and Merge() return interface{} types. After go
generics are available, these will be typed.

Introduce generated getters for all fields of expconf structs, which are
correctly typed according to whether a defaulted object will have fields
defaulted or not.
  • Loading branch information
rb-determined-ai authored Mar 10, 2021
1 parent cfa00f7 commit 8e07c66
Show file tree
Hide file tree
Showing 26 changed files with 1,128 additions and 1,262 deletions.
10 changes: 5 additions & 5 deletions common/Makefile
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
GENERATED_COMMITTED := determined_common/schemas/expconf/_gen.py
GENERATION_INPUTS = ../schemas/gen.py $(shell find ../schemas/expconf -name '*.json')

.PHONY: clean
clean:
Expand Down Expand Up @@ -27,11 +28,10 @@ check-gen: force-gen $(GENERATED_COMMITTED)
# git reports the files as unchanged after forcibly regenerating the files:
test -z "$(shell git status --porcelain $(GENERATED_COMMITTED))"

determined_common/schemas/expconf/_gen.py: ../schemas/gen.py $(shell find ../schemas/expconf -name "*.json")
../schemas/gen.py \
--output $@ \
python \
$(shell find ../schemas/expconf -name "*.json")
determined_common/schemas/expconf/_gen.py: $(GENERATION_INPUTS)
../schemas/gen.py python\
--package expconf \
--output $@

.PHONY: build
build:
Expand Down
1 change: 0 additions & 1 deletion common/determined_common/schemas/expconf/_gen.py
Original file line number Diff line number Diff line change
Expand Up @@ -2816,4 +2816,3 @@
"""
),
}

5 changes: 2 additions & 3 deletions master/.golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,10 @@ run:
# The exit code when at least one issue was found.
issues-exit-code: 1

# Exclude generated code.
skip-files:
- pkg/schemas/schema_gen.go
- pkg/schemas/expconf/schema_gen.go
- pkg/schemas/expconf/dummies.go
- pkg/schemas/*/zgen*.go
- pkg/schemas/zgen*.go

output:
# Linter output format.
Expand Down
37 changes: 12 additions & 25 deletions master/Makefile
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
GENERATED_COMMITTED := pkg/schemas/schema_gen.go pkg/schemas/expconf/schema_gen.go
GENERATED := $(GENERATED_COMMITTED) packaging/LICENSE
GENERATED := packaging/LICENSE
GENERATION_INPUTS = ../schemas/gen.py $(shell find ./pkg/schemas/ -name '*.go') $(shell find ../schemas/expconf -name '*.json')

export VERSION := $(shell cat ../VERSION)
export GO111MODULE := on
Expand All @@ -12,20 +12,25 @@ clean: ungen
.PHONY: ungen
ungen:
rm -f $(GENERATED)
git checkout -- $(GENERATED_COMMITTED)
rm -f `find ./pkg/schemas/ -name 'zgen_*.go'` build/schema_gen.stamp

.PHONY: gen
gen: $(GENERATED)
gen: $(GENERATED) build/schema_gen.stamp

.PHONY: force-gen
force-gen:
touch ../schemas/gen.py
rm -f build/schema_gen.stamp

build/schema_gen.stamp: $(GENERATION_INPUTS)
go generate ./pkg/schemas/...
mkdir -p build
touch $@

.PHONY: check-gen
check-gen: force-gen $(GENERATED_COMMITTED)
check-gen: force-gen gen
# Checking that committed, generated code is up-to-date by ensuring that
# git reports the files as unchanged after forcibly regenerating the files:
test -z "$(shell git status --porcelain $(GENERATED_COMMITTED))"
test -z "$(shell git status --porcelain ./pkg/schemas)"

.PHONY: get-deps
get-deps:
Expand Down Expand Up @@ -105,21 +110,3 @@ publish-dev:

packaging/LICENSE: $(shell find ../tools/scripts/licenses -type f)
../tools/scripts/gen-attributions.py master $@

# Root schemas generated file; this contains all the schemas we have.
pkg/schemas/schema_gen.go: ../schemas/gen.py $(shell find ../schemas -regex ".*/v.*/.*\.json")
../schemas/gen.py \
--output $@ \
--package schemas \
go \
$(shell find ../schemas -regex ".*/v.*/.*\.json")
goimports -l -local github.com/determined-ai -w $@

# Package-level generated file; this only generates code related to expconf objects.
pkg/schemas/expconf/schema_gen.go: ../schemas/gen.py $(shell find ../schemas/expconf -name "*.json")
../schemas/gen.py \
--output $@ \
--package expconf \
go \
$(shell find ../schemas/expconf -name "*.json")
goimports -l -local github.com/determined-ai -w $@
76 changes: 76 additions & 0 deletions master/pkg/schemas/copy.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
package schemas

import (
"fmt"
"reflect"
)

// cpy is for deep copying, but it will only work on "nice" objects, which should include our
// schema objects. Useful to other reflect code.
func cpy(v reflect.Value) reflect.Value {
// fmt.Printf("cpy(%T)\n", v.Interface())
var out reflect.Value

switch v.Kind() {
case reflect.Ptr:
if v.IsZero() {
return v
}
out = reflect.New(v.Elem().Type())
out.Elem().Set(cpy(v.Elem()))

case reflect.Interface:
if v.IsZero() {
return v.Elem()
}
out = cpy(v.Elem())

case reflect.Struct:
out = reflect.New(v.Type()).Elem()
// Recurse into each field of the struct.
for i := 0; i < v.NumField(); i++ {
out.Field(i).Set(cpy(v.Field(i)))
}

case reflect.Map:
typ := reflect.MapOf(v.Type().Key(), v.Type().Elem())
if v.IsZero() {
// unallocated map
out = reflect.Zero(typ)
} else {
out = reflect.MakeMap(typ)
// Recurse into each key of the map.
for _, key := range v.MapKeys() {
val := v.MapIndex(key)
out.SetMapIndex(key, cpy(val))
}
}

case reflect.Slice:
typ := reflect.SliceOf(v.Type().Elem())
if v.IsZero() {
// unallocated slice
out = reflect.Zero(typ)
} else {
out = reflect.MakeSlice(typ, 0, v.Len())
// Recurse into each element of the slice.
for i := 0; i < v.Len(); i++ {
val := v.Index(i)
out = reflect.Append(out, cpy(val))
}
}

// Assert that none of the "complex" kinds are present.
case reflect.Array,
reflect.Chan,
reflect.Func,
reflect.UnsafePointer:
panic(fmt.Sprintf("unable to cpy %T of kind %v", v.Interface(), v.Kind()))

default:
// Simple types like string or int can be passed directly.
return v
}

return out
}
82 changes: 82 additions & 0 deletions master/pkg/schemas/copy_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
package schemas

import (
"reflect"
"testing"

"gotest.tools/assert"
)

// Copy is the non-reflect version of copy, but mostly the reflect version is called from other
// reflect code, so it's defined here in test code.
func Copy(src interface{}) interface{} {
return cpy(reflect.ValueOf(src)).Interface()
}

func TestCopyAllocatedSlice(t *testing.T) {
src := []string{}
obj := Copy(src).([]string)
assert.DeepEqual(t, obj, src)
}

func TestCopyUnallocatedSlice(t *testing.T) {
// Copying an unallocated slice encodes to null.
var src []string
obj := Copy(src).([]string)
assert.DeepEqual(t, obj, src)
}

func TestCopyAllocatedMap(t *testing.T) {
// Copying an allocated map encodes to [].
src := map[string]string{}

obj := Copy(src).(map[string]string)
assert.DeepEqual(t, obj, src)
}

func TestCopyUnallocatedMap(t *testing.T) {
// Copying an unallocated map encodes to null.
var src map[string]string

obj := Copy(src).(map[string]string)
assert.DeepEqual(t, obj, src)
}

type A struct {
M map[string]string
S []int
B B
}

type B struct {
I int
S string
C []C
}

type C struct {
I int
D map[string]D
}

type D struct {
I int
S string
}

func TestCopyNested(t *testing.T) {
src := A{
M: map[string]string{"eeny": "meeny", "miney": "moe"},
S: []int{1, 2, 3, 4},
B: B{
I: 5,
S: "five",
C: []C{
{I: 6, D: map[string]D{"one": {I: 1, S: "fish"}, "two": {I: 2, S: "fish"}}},
{I: 6, D: map[string]D{"red": {I: 3, S: "fish"}, "blue": {I: 4, S: "fish"}}},
},
},
}
obj := Copy(src).(A)
assert.DeepEqual(t, obj, src)
}
Loading

0 comments on commit 8e07c66

Please sign in to comment.