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

handle headers and params correctly #688

Merged
merged 1 commit into from Mar 4, 2020
Merged

Conversation

rpatali
Copy link
Contributor

@rpatali rpatali commented Mar 4, 2020

  • params are getting reordered between check-generate because we are
    iterating over a map which doesn't gaurantee ordering. Using a list of
    keys to preserve iteration order fixes that
  • non-top level headers generate completely meaningless code by having a
    series of empty if checks and the actual logic that is supposed to gated
    by the checks are actually generated outside of the if check.

- params are getting reordered between check-generate because we are
iterating over a map which doesn't gaurantee ordering. Using a list of
keys to preserve iteration order fixes that
- non-top level headers generate completely meaningless code by having a
series of empty if checks and the actual logic that is supposed to gated
by the checks are actually generated outside of the if check.
@@ -448,6 +448,12 @@ func (c *barClient) ArgWithHeaders(
headers["x-uuid"] = string(*r.UserUUID)
}

if r.ParamsStruct != nil {
Copy link
Contributor Author

@rpatali rpatali Mar 4, 2020

Choose a reason for hiding this comment

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

before the fix, this would have been generated as:

	if r.ParamsStruct != nil {
		if r.ParamsStruct.UserID != nil {
		}
	}
        headers["user-id"] = string(*r.ParamsStruct.UserID)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 69.854% when pulling ad039b3 on rpatali/header-params into cb342ff on master.

Copy link
Contributor

@tejaswiagarwal tejaswiagarwal left a comment

Choose a reason for hiding this comment

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

Minor comment, the rest looks good. Thanks for adding the test.

@@ -552,17 +553,18 @@ func findStructs(
}

seenStructs[longFieldName] = typeName
itrOrder = append(itrOrder, longFieldName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment on why we are doing this? i.e. Ordering problems with maps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

does it really need a comment? The var is already itrOrder :)

@rpatali rpatali merged commit 5940c66 into master Mar 4, 2020
@rpatali rpatali deleted the rpatali/header-params branch March 4, 2020 21:04
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.

None yet

3 participants