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

Refactor #186

Merged
merged 8 commits into from
Dec 20, 2024
Merged

Refactor #186

merged 8 commits into from
Dec 20, 2024

Conversation

bhornseth
Copy link
Member

No description provided.

Each of these changes were causing a longer version of the below warning

> WARNING: Using the `raise_error` matcher without providing a specific error
> or message risks false positives

There are definitely better refactors for each of these, but I'm
optimizing for cleaning up the test output, not for improving the code
interface or possibly introducing breaking changes
 - Add support for Ruby 3.2 and 3.3
 - Require Faraday 2.0+
This change turns off request and response logging by default and allows
users to turn that back on by setting a new `verbose` option to true.
@@ -1,3 +1,10 @@
v1.7.0
- Drop support for ruby 2.7
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -31,10 +31,9 @@ Gem::Specification.new do |s|
s.require_paths = ["lib"]

s.add_dependency 'addressable'
s.add_dependency 'faraday', '>= 0.17.3', '< 2.0'
s.add_dependency 'multi_json', '~> 1.0'
s.add_dependency 'faraday', '>= 2.0.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@wlmcewen wlmcewen left a comment

Choose a reason for hiding this comment

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

Good long overdue changes. Nothing stands out to me, but perhaps the verbose option could be back ported if needed (I'm hoping that's unnecessary).

 - only build on some pushes, but all pull requests
 - support manual builds via workflow_dispatch
 - change publish to be triggered by a GitHub release action
 - update to latest actions/checkout script
bhornseth and others added 4 commits December 11, 2024 13:45
This change removes the `compress` option from the client entirely.

We use the default faraday adapter, which is `net/http` from stdlib. If
you read [the documentation for that module][net-http-docs], you'll come
across this:

> `Net::HTTP` automatically adds Accept-Encoding for compression of
> response bodies and automatically decompresses gzip and deflate
> responses unless a Range header was sent.

In other words, by default, ruby's net/http module will request gzipped
content and automatically uncompress it if that's what the response
contains.

If instead one sets the `Accept-Encoding` header on a request that's
made with `net/http`, that default behavior is ignored and we're left to
deal with unpacking hte response ourselves as was done in the
`FaradayMiddleware` class.

In other words, whether the `compress` option is used or not, the
behavior has been that we're asking for compressed responses and the
option is effectively ignored. I went back to ruby 1.9.2 and the same
behavior existed there. Given that, I'm confident this option has always
been either ignored or redundant.

[net-http-docs]: https://ruby-doc.org/stdlib-3.0.0/libdoc/net/http/rdoc/Net/HTTP.html#class-Net::HTTP-label-Compression
Highline is only used within this cli provider, and only to capture and
do some minor formatting on the user input. This changeset removes
highline entirely and replaces it with equivalent ruby
The new Faraday::Middleware class does not have a `body` method from the response and requires us to retrieve it from the `env` variable.
@bhornseth bhornseth merged commit 0c8b5df into master Dec 20, 2024
4 checks passed
@bhornseth bhornseth deleted the refactor branch December 20, 2024 13:37
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.

3 participants