smartystreets / smartystreets-go-sdk

The official client libraries for accessing SmartyStreets APIs from Go.
https://smartystreets.com/docs/sdk/go
Apache License 2.0
20 stars 6 forks source link

Support for Autocomplete API #1

Closed edwinthinks closed 8 years ago

edwinthinks commented 8 years ago

Hello,

My team is aiming to use this library to communicate with the Autocomplete API service. Unfortunately, it doesn't seem to be currently supported by the library. I was hoping to get some advice and suggestions on implementing support for Autocomplete in this library so that it can merge into a release down the line.

I've made some changes that allowed support for this. I would like your feedback on some of these decisions I've made.

1) Added us-autocomplete-api and copied client.go from us-street-api but changed buildRequest() to build a GET request instead.

request, _ := http.NewRequest("GET", "/suggest", nil)
// Apply parameters using request.Url.Query()

It seemed like this change caused func (r *RetryClient) Do(request *http.Request) to failing at line body, err := ioutil.ReadAll(request.Body) with:

panic: runtime error: invalid memory address or nil pointer dereference [recovered]
    panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x810b4]

I fixed this issue by encapsulating request.Body related code in a conditional:

    if request.Method == "POST" {
        body, err := ioutil.ReadAll(request.Body) // TODO: check err and bail if non-nil
        if err != nil {
            return nil, err
        }
        request.Body = ioutil.NopCloser(bytes.NewReader(body))
    }

After these changes, I was able to build a client able to perform the Autocomplete API call

2) I disbanded the use of Batch because this api doesn't support bulk requests. I modified this new client to send a single struct *autocomplete.Lookup and save the suggestions/results in autocomplete.Lookup public field Result

Please let me know of any suggestions or feedback. Hope to be able to contribute. Thanks!

mdwhatcott commented 8 years ago

@cintosyntax - We've actually had plans to fill in clients and data structures for the remaining APIs but have been implementing the SDK in other languages to achieve a baseline of support across various platforms and technologies.

However, now that I know that you're hoping to use the us-autocomplete-api soon I will expedite implementing a client for that API. As far as an estimated timeline for a fully functional, unit-tested us-autocomplete-api client, complete with all wireup/builder code (that we would feel comfortable supporting for the long-term), here's what I'm thinking:

Best Case: Done by Friday Oct. 21, 2016 Probable Case: Done by Wednesday, Oct. 26, 2016 Worst Case: Done by Friday Nov. 4, 2016

There is, of course, a small chance that it will take me longer to finish, but my estimate above (which isn't a firm commitment) accurately represents my current level of certainty (or uncertainty?) given my other commitments and endeavors. I will post additional updates here if my understanding of the timeline changes.

I'd be happy to look over your changes and consider merging some of all of what you've done. Maybe the best way to share the code is with a pull request?

I'll start working in the us-autocomplete-api branch.

mdwhatcott commented 8 years ago

smartystreets-go-sdk: client for us-autocomplete-api

mdwhatcott commented 8 years ago

I've implemented most of the code. it probably looks very close to what you've done. Aside from fixing the RetryClient it's almost done. My initial estimate appears overly cautious that this point.

mdwhatcott commented 8 years ago

RetryClient is finished. Just a few loose ends to tie up in the client. Will probably publish new version tomorrow.

edwinthinks commented 8 years ago

Thanks! Excited to see this merged into master =)

mdwhatcott commented 8 years ago

@cintosyntax - As of 7f90880b the us-autocomplete-api branch is ready for real testing. Let me know if it meets your needs. When you're comfy with it we'll merge to master and publish a new release/tag.