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

fix(neuron.connection.http): fix error case so return value matches t… #60

Merged
merged 3 commits into from
Jun 10, 2020

Conversation

krisalyssa
Copy link
Contributor

…ypespec

The definition of the JSONParseError struct says that the response element is a
Neuron.Response.t(), but the code was actually returning a tuple ({:error, Neuron.Response.t()}). With this change the type of response matches the typespec.

BREAKING CHANGE: If your code expects the response field of JSONParseError to be a tuple, it
will break.

fix #59

…ypespec

The definition of the `JSONParseError` struct says that the `response` element is a
`Neuron.Response.t()`, but the code was actually returning a tuple (`{:error,
Neuron.Response.t()}`). With this change the type of `response` matches the typespec.

BREAKING CHANGE: If your code expects the `response` field of `JSONParseError` to be a tuple, it
will break.

fix uesteibar#59
@uesteibar
Copy link
Owner

Hi there! thanks for taking care of this ❤️ however there is some tests failing that we should fix first, you can see it on https://travis-ci.org/github/uesteibar/neuron/builds/685800174.

Not sure why it's not showing in here, I'll investigate

@krisalyssa
Copy link
Contributor Author

I'm finally circling back to check on these failing tests. The first thing I notice is that TravisCI is building using older versions of Elixir and Erlang than I did when developing, and it looks like something's trying to use a function that's not available in the older Erlang. I'm installing OTP 19.0 and Elixir 1.6.6 to see if and how I need to adjust my PR.

@krisalyssa
Copy link
Contributor Author

@uesteibar I was unable to get OTP 19.0 to install successfully on macOS 10.15, so I can't recreate the error that TravisCI is reporting. So I'm making educated guesses based on what I can see in TravisCI, and editing the code in the PR to what I hope will be more OTP 19-friendly. There may be several code pushes until I get it right, since the only way I can tell is to have TravisCI run the tests here.

The test added in the previous commit for this PR was developed against OTP, and apparently contains
code which isn't compatible with OTP 19 (with which TravisCI runs the tests).

fix uesteibar#59
@krisalyssa
Copy link
Contributor Author

That push appears to have fixed the tests.

Copy link
Owner

@uesteibar uesteibar left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this! just a tiny thing, after that I'll merge and release 👏 ❤️

lib/neuron/connection/http.ex Outdated Show resolved Hide resolved
Move `build_response/1` to after `build_response_tuple/1` since the latter calls the former

fix uesteibar#59
@uesteibar uesteibar merged commit b96d871 into uesteibar:master Jun 10, 2020
@krisalyssa krisalyssa deleted the csc/issue-59 branch June 10, 2020 14:00
@uesteibar
Copy link
Owner

Published as v5.0.0 🚀 thank you!

@krisalyssa
Copy link
Contributor Author

You're very welcome! neuron has been useful to us, so I'm happy to give something back.

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.

Bug in returning a JSONParseError
2 participants