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.

Sign up for GitHub

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

Jump to bottom

Replace JSON parser with YAML parser for *-configs #468

Merged
merged 1 commit into from
Sep 28, 2018
Merged

Replace JSON parser with YAML parser for *-configs #468

merged 1 commit into from
Sep 28, 2018

Conversation

l-w-2017
Copy link
Contributor

No description provided.

@coveralls
Copy link

coveralls commented Sep 19, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.04%) to 61.569% when pulling b067e04 on yaml_for_configs into a5bc910 on master.

jacobgreenleaf
jacobgreenleaf suggested changes Sep 20, 2018
View reviewed changes
codegen/module.go
// JSONFileRaw is the raw JSON file read as bytes used for future parsing
JSONFileRaw []byte
// The YAMLFileName is file name of the instance YAML file
YAMLFileName string
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a breaking change and there is code in multiple projects that is already written using JSONFileName. You should mark the current field as deprecated using // Deprecated: comment and when creating this object you have the new field like ConfigFileName instead.

codegen/module.go Outdated
// JSONClassConfig maps onto a json configuration for a class type
type JSONClassConfig struct {
// YAMLClassConfig maps onto a YAML configuration for a class type
type YAMLClassConfig struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is technically a breaking change but I don't see anyone using it so I'm fine. If you can try to make it unexported instead of exported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

YAMLClassConfig -> yamlClassConfig

codegen/module.go Outdated
// Name is the class instance name used to identify the module as a
// dependency. The combination of the class Name and this instance name
// is unique.
Name string `json:"name"`
Name string `yaml:"name"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than replacing these annotations you should write yaml:"name" json:"name" so that existing code that decodes into these structs can do so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

json annotation added for all exposed structs

codegen/post_gen_hooks.go Outdated
return errors.Wrapf(
err,
"error parsing client-config.json for client %q",
"error parsing client-config.yaml for client %q",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Since it could be either client-config.json or client-config.yaml you should use the import path here rather than hard coding it.

codegen/module.go
@@ -698,76 +698,76 @@ func (system *ModuleSystem) readInstance(
instanceDirectory string,
) (*ModuleInstance, error) {

jsonFileName := className + configSuffix
yamlFileName := className + configSuffix
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should be looking for both foo-config.yaml and foo-config.json, preferring the .yaml file if it exists, otherwise falling back to .json for backwards compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"configSuffix" is still "json". In the future diff, we will create a helper function to get the correct fileName for both json and yaml.

codegen/template.go Outdated
@@ -60,7 +60,7 @@ var defaultFuncMap = tmpl.FuncMap{
"dec": decrement,
"basePath": filepath.Base,
"pascal": PascalCase,
"jsonMarshal": jsonMarshal,
"yamlMarshal": yamlMarshal,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: from what I could tell this function is unused in the template.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

function removed

@l-w-2017 l-w-2017 force-pushed the yaml_for_configs branch 2 times, most recently from 0ca58f7 to ea89ee4 Compare September 24, 2018 21:55
jacobgreenleaf
jacobgreenleaf reviewed Sep 27, 2018
View reviewed changes
codegen/gateway.go Outdated
f,
)
}
}

clientSpec := &ClientSpec{
JSONFile: instance.JSONFileName,
Copy link
Contributor

Choose a reason for hiding this comment

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

There is existing code that relies on JSONFile property. Can you keep the existing property and add YAMLFile as a new property?

jacobgreenleaf
jacobgreenleaf approved these changes Sep 28, 2018
View reviewed changes
codegen/module.go
Dependencies: dependencies,
ResolvedDependencies: map[string][]*ModuleInstance{},
RecursiveDependencies: map[string][]*ModuleInstance{},
DependencyOrder: []string{},
JSONFileName: jsonFileName,
Copy link
Contributor

Choose a reason for hiding this comment

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

Keep JSONFileRaw

codegen/module.go
JSONFileRaw []byte
// The JSONFileName is file name of the instance JSON file
JSONFileName string // Deprecated
// The YAMLFileName is file name of the instance YAML file
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing JSONFileRaw

codegen/post_gen_hooks.go Outdated
@@ -40,22 +40,22 @@ const (

// helper struct to pull out the fixture config
type moduleConfig struct {
Config *config `json:"config"`
Config *config `yaml:"config"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing json tags

@l-w-2017 l-w-2017 closed this Sep 28, 2018
@l-w-2017 l-w-2017 reopened this Sep 28, 2018
@l-w-2017 l-w-2017 merged commit 8f952a9 into master Sep 28, 2018
@l-w-2017 l-w-2017 deleted the yaml_for_configs branch September 28, 2018 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jacobgreenleaf jacobgreenleaf jacobgreenleaf approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@l-w-2017 @coveralls @jacobgreenleaf