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

Expect short names in flatten functions #63

Open
houkms opened this issue Oct 17, 2019 · 5 comments
Open

Expect short names in flatten functions #63

houkms opened this issue Oct 17, 2019 · 5 comments
Assignees
Labels
new code style Need code style is needed

Comments

@houkms
Copy link
Collaborator

houkms commented Oct 17, 2019

Auto-Gened:

if natGatewayPropertiesFormat := resp.NatGatewayPropertiesFormat; natGatewayPropertiesFormat != nil {

Expected:

if props:= resp.NatGatewayPropertiesFormat; props != nil {   
@houkms
Copy link
Collaborator Author

houkms commented Oct 17, 2019

I expect it's only the root property in Read function need such modifications.

@houkms houkms added the new code style Need code style is needed label Oct 17, 2019
@magodo
Copy link
Collaborator

magodo commented Nov 12, 2019

I expect it's only the root property in Read function need such modifications.

What if there is also nested object inside another nested object? E.g. we have following nested objects:

type Foo struct {
	*Bar
}

type Bar struct {
	*Baz
}

type Baz struct {
	Name string
}

func main() {
	f := &Foo{
		&Bar{
			&Baz{
				Name: "foobar",
			},
		},
	}

	if prop := f.Bar; prop != nil {
                // `prop` below has different identity
		if prop := prop.Baz; prop != nil {
			fmt.Println(prop.Name)
		}
	}
}

Take note at the comment above, where prop has different identity in one line. Does this kind of style introduce other issue?

@houkms
Copy link
Collaborator Author

houkms commented Nov 12, 2019

I suppose this case will not appear. This issue is essentially a naming convention of Hashicorp, specifically, when using client.response to set the go SDK filed, they require:

  • for the root property: it appear directly in Read function, and we need to use a tmp variable named props to accept the response values.
  • for the child properties: they appear whether in the Read function or flatten function, and we can use the go_filed_name as the tmp variable name.

MM now always uses go_field_name as the tmp variable name to accept the response values, which is what Hashicorp complains about.

For your specific example. it should be in this way (because Baz is a nestedOject, we need a flatten function).

if prop := f.Bar; prop != nil {
     // `prop` below has different identity
    if err := d.set("Baz", flattenBaz(prop.Baz)){
        return fmt.Error(" ... ")
    }
}

If Baz is not a nestedObject, we can do it in this way

if prop := f.Bar; prop != nil {
    // `prop` below has different identity
    if baz := prop.Baz; baz != nil {
        return fmt.Error(" ... ")
    }
}

@magodo
Copy link
Collaborator

magodo commented Nov 13, 2019

After digging further, I found that part of naming is controled by file: nested_object_shcema_assign.erb, as below:

<%
    ...
    temp_var = sdk_type.go_variable_name || sdk_type.go_field_name.camelcase(:lower) || input
-%>
...

And this part is only used for this purpose. From the code above we can see the var name is resolved via prioprity: go_variable_name > go_field_name > input (input is a passed in argument, e.g., 'resp')

So I suppose for this issue, we can just modify the read::response section in terraform.yaml, adding go_variable_name: properties to /properties field.

@magodo
Copy link
Collaborator

magodo commented Nov 14, 2019

Update last reply

If we want the embeded part of unmarshalling(i.e. sdk -> schema) to follow same naming convention on tmp var, we need to apply the same naming convention used in nested_object_schema_assign.erb in other places.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new code style Need code style is needed
Projects
None yet
Development

No branches or pull requests

2 participants