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

Allows plain body to be set, rather than just JSON or Form variables #9

Closed
wants to merge 1 commit into from

Conversation

andyjeffries
Copy link
Contributor

Hi Dalton,

I wanted to use https://github.com/Jeffail/gabs with Sling, but found that you don't allow setting the body manually, only from an existing JSON-marshallable structure or form data. Here's a PR that allows you to manually set the body.

I'm pretty new to Go, so if I've done anything daft feel free to let me know and I'll amend it.

Cheers,

Andy

@dghubble
Copy link
Owner

Hey thanks for the PR.

Sling separates building a request (with various boilerplate setters, culminating in a call to Request() (*http.Request, error)) and executing the request (with Do(req *http.Request, success, failure interface{})) to allow developers to make further modifications to the Request before executing.

You can already set a plain body on the http.Request directly.

req, err := sling.New().Base("http://example.com/").Path("foo").Request()
req.Body = ioutil.NopCloser(strings.NewReader(expectedBody))

A test to that effect:

func TestCustomBodySetter(t *testing.T) {
    expectedBody := "string body"
    req, err := New().Base("http://example.com/").Path("foo").Request()
    if err != nil {
        t.Errorf("request could not be created")
    }
    req.Body = ioutil.NopCloser(strings.NewReader(expectedBody))
    buf := new(bytes.Buffer)
    buf.ReadFrom(req.Body)
    if value := buf.String(); value != expectedBody {
        t.Errorf("expected %s, got %s", expectedBody, value)
    }
}

You're right though, there is a reasonable case for a Body setter to compliment BodyForm and BodyJSON so the Receive and ReceiveSuccess convenience methods can be used.

However, its signature should be func (*Sling) Body(body io.Reader) *Sling because a string body is just one particular possibility. http.NewRequest is a useful example

@andyjeffries
Copy link
Contributor Author

Thanks for the helpful reply/comment.

I've changed the commit and pushed the new version with the correct signature. Is that what you require? Have I done it the right way?

@andyjeffries
Copy link
Contributor Author

Actually the docs say that strings.NewReader is more efficient than bytes.NewBufferString, but for the test I guess this doesn't matter. I got bytes.NewBufferString from a mailing list post as the recommend way, but if you want me to change it, let me know.

@@ -33,6 +33,8 @@ type Sling struct {
bodyJSON interface{}
// url tagged body struct (form)
bodyForm interface{}
// simply assigned body
body string
Copy link
Owner

Choose a reason for hiding this comment

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

// plain body
body io.ReadCloser

@andyjeffries
Copy link
Contributor Author

Thanks for the Dalton, I appreciate you taking the time to help me make this better. Let's see if this version is OK? I think TestBodySetter could be better (I'm not a fan of the empty string switching it with nil), but I couldn't figure out a better way.

@dghubble
Copy link
Owner

Ah, I see the problem. If we make a separate

var testInput = ioutil.NopCloser(strings.NewReader("test"))

and use io.Reader or io.ReadCloser directly, the empty string switching can be avoided.

I tweaked that little bit and merged as 3b289d0 Thanks! Hope you're enjoying Go!

@dghubble dghubble closed this Sep 14, 2015
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