Skip to content
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

Embeded structs fix tests part2 #108

Open
wants to merge 4 commits into
base: feature/embeded-structs
Choose a base branch
from

Conversation

skimata
Copy link

@skimata skimata commented Aug 3, 2017

No description provided.

Copy link
Contributor

@shwoodard shwoodard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Update all table tests, in this pr and elsewhere already merged to use t.Run, subtests
  • In general you have a lot of functions that could be inlined because they are too short or used only once

}

for _, scenario := range tests {
t.Logf("scenario: %s\n", scenario.name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use t.Run. See "Table-driven tests using subtests" @ https://blog.golang.org/subtests

// * adds new entries as-is
// * tracks dominant field conflicts
func (a attributes) set(k string, v interface{}) {
if val, ok := a[k]; ok {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename ok to hasKey. Convention is to use ok for type assertion only.

})
}

func (n *Node) mergeFunc(node *Node, attrSetter func(attrs attributes)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move mergeFunc higher in the file as it used by methods before it is declared.

}
}

// mergeAttributes consolidates multiple nodes into a single consolidated node
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment doesn't seem to make sense here: seems to be for a different function.

@@ -183,3 +239,36 @@ func deepCopyNode(n *Node) *Node {
}
return &copy
}

// nodeError is used to track errors in processing Node related values
type nodeError interface {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there more than one of these things? Why do we need an interface?

// dominantFieldConflict is a specific type of marshaling scenario that the standard json lib treats as an error
// comment from json.dominantField(): "If there are multiple top-level fields...This condition is an error in Go and we skip"
// tracking the key and conflicting values for future use to possibly surface an error
type dominantFieldConflict struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename embeddedStructFieldConflict

var er error

modelValue := reflect.ValueOf(model).Elem()
modelType := reflect.ValueOf(model).Type().Elem()

node := new(Node)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Identify if is embedded and extract embedded extraction to own func

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants