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

Fixup for 2e83b0dfdd45e1e54615606924dca565b547b08b #900

Merged
merged 2 commits into from Nov 21, 2016

Conversation

Projects
None yet
2 participants
@kwk
Copy link
Contributor

kwk commented Nov 21, 2016

We are using the v1 branch and lately there seems to be a change about the contentType parameter. I've commented on the recent commit here.

This change removes the parameter contentType parameter from a function call under the same conditions as introduced in 2e83b0d.

Also, I've added a workaround to remove the header declared and not used error which was caused because no content type is being anymore.

Fixup for 2e83b0d
We are using the v1 branch and lately there seems to be a change about the `contentType` parameter. I've commented on the recent commit [here](2e83b0d#commitcomment-19905560). 

This change removes the parameter `contentType` parameter from a function call under the same conditions as introduced in 2e83b0d.

Also, I've added a workaround to remove the `header declared and not used error` which was caused because no content type is being anymore.

kwk referenced this pull request Nov 21, 2016

Only generate "contentType" argument for clent methods when necessary
That is only if API supports multiple inbound content types.
@@ -990,6 +990,7 @@ func (c *Client) {{ $funcName }}(ctx context.Context, path string{{ if .Params }
return nil, err
}
{{ if or .Headers .HasPayload }} header := req.Header
_ = header // Avoid "header declared and not used" error
{{ if .HasPayload }}{{ if .HasMultiContent }} if contentType != "*/*" {

This comment has been minimized.

@raphael

raphael Nov 21, 2016

Member

It seems moving the check for HasMultiContent might be better, something like:

{{ if or .Headers (and .HasPayload .HasMultiContent) }}	header := req.Header
{{ if and .HasPayload .HasMultiContent }}	if contentType != "*/*" {

The idea being that we only need to set headers if there's a payload and a contentType parameter or if there are headers to be set. The code above also combines the two if on line 994 into one which means one of the end below (on line 997) must be removed. Thank you!

This comment has been minimized.

@kwk

kwk Nov 21, 2016

Contributor

@raphael Fixed in 762a496

@raphael

This comment has been minimized.

Copy link
Member

raphael commented Nov 21, 2016

Thank you! I added a comment in the commit, do you think you could make that change? and once you've made it could you also make a PR against branch v1. Thank you again, much appreciated!

@raphael

This comment has been minimized.

Copy link
Member

raphael commented Nov 21, 2016

Thank you!

@raphael raphael merged commit c72c2cd into goadesign:master Nov 21, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

kwk added a commit to kwk/goa that referenced this pull request Nov 21, 2016

Fixup for 2e83b0d (goadesign#900)
* Fixup for 2e83b0d

We are using the v1 branch and lately there seems to be a change about the `contentType` parameter. I've commented on the recent commit [here](goadesign@2e83b0d#commitcomment-19905560). 

This change removes the parameter `contentType` parameter from a function call under the same conditions as introduced in 2e83b0d.

Also, I've added a workaround to remove the `header declared and not used error` which was caused because no content type is being anymore.

* Create header if .HasPayload .HasMultiContent 

See goadesign#900 (review) for change request.

@kwk kwk deleted the kwk:patch-2 branch Nov 21, 2016

kwk added a commit to kwk/goa that referenced this pull request Nov 21, 2016

Fixup for 2e83b0d (goadesign#900)
* Fixup for 2e83b0d

We are using the v1 branch and lately there seems to be a change about the `contentType` parameter. I've commented on the recent commit [here](goadesign@2e83b0d#commitcomment-19905560). 

This change removes the parameter `contentType` parameter from a function call under the same conditions as introduced in 2e83b0d.

Also, I've added a workaround to remove the `header declared and not used error` which was caused because no content type is being anymore.

* Create header if .HasPayload .HasMultiContent 

See goadesign#900 (review) for change request.

@kwk kwk referenced this pull request Nov 21, 2016

Merged

Backport for #900 #903

@kwk

This comment has been minimized.

Copy link
Contributor

kwk commented Nov 21, 2016

@raphael

[...] and once you've made it could you also make a PR against branch v1.

Here's the PR for v1: #903

raphael added a commit that referenced this pull request Nov 21, 2016

Fixup for 2e83b0d (#900) (#903)
* Fixup for 2e83b0d

We are using the v1 branch and lately there seems to be a change about the `contentType` parameter. I've commented on the recent commit [here](2e83b0d#commitcomment-19905560). 

This change removes the parameter `contentType` parameter from a function call under the same conditions as introduced in 2e83b0d.

Also, I've added a workaround to remove the `header declared and not used error` which was caused because no content type is being anymore.

* Create header if .HasPayload .HasMultiContent 

See #900 (review) for change request.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment