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

Backport a ton of changes #22

Merged
merged 18 commits into from
Nov 16, 2023
Merged

Backport a ton of changes #22

merged 18 commits into from
Nov 16, 2023

Conversation

whatyouhide
Copy link
Contributor

Hi @Jcambass! 👋

I’m using this library in Xandra, a Cassandra/ScyllaDB driver I maintain. I'm using a fork of this since I needed a bunch of things to be fixed, such as making sure proxies and toxics get destroyed after apply!/2 and things like that.

I don't have the time right now to do all these commits as nice tidy PRs to this upstream, but I figured I'd dump it here in case you wanna merge this in. The commits are split up nicely, so you should be able to review them individually.

Cheers!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added Dialyzer and linting to CI.

Comment on lines +1 to +2
erlang 26.0.2
elixir 1.15.4-otp-26
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modernized this

defexception message: "Server Error"

@impl true
def exception(options) when is_list(options) do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The error message is significantly more helpful now

@Jcambass Jcambass self-assigned this Oct 20, 2023
Copy link
Owner

@Jcambass Jcambass left a comment

Choose a reason for hiding this comment

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

Thanks a lot for all these improvements! ❤️

As far as the changes themself, I agree with your choices 👍🏻

I will keep this open for a moment until I had to chance to run it myself and give it a second throughout review.

Copy link
Contributor Author

@whatyouhide whatyouhide left a comment

Choose a reason for hiding this comment

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

@Jcambass sound good, thank you. I added a few comments to make the re-review easier. No rush on my end, I’m using this from my fork directly in Xandra, since it's a test-only dependency anyways 🙃

.github/workflows/main.yml Show resolved Hide resolved
.tool-versions Show resolved Hide resolved
lib/toxiproxy_ex.ex Show resolved Hide resolved
lib/toxiproxy_ex.ex Show resolved Hide resolved
lib/toxiproxy_ex/client.ex Show resolved Hide resolved
lib/toxiproxy_ex/client.ex Show resolved Hide resolved
lib/toxiproxy_ex.ex Show resolved Hide resolved
@Jcambass Jcambass merged commit 6cb3563 into Jcambass:main Nov 16, 2023
6 checks passed
@whatyouhide
Copy link
Contributor Author

Woooohooooo thanks @Jcambass! Let me know if you release a new version 🙃

@Jcambass
Copy link
Owner

Thank you! I'll let you know once I've written the changelog and publish a new version.

@Jcambass
Copy link
Owner

Jcambass commented Jul 3, 2024

Apologise for the delay. I published a new version in February but forget to comment here 🙃

https://hex.pm/packages/toxiproxy_ex

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