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

Drop depenency on base64 #778

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Earlopain
Copy link

#759 added base64 as a depenency in order to suppress a warning/error on Ruby 3.3/3.4

Since base64 is mostly just a wrapper around the pack/unpack methods it's easy to just replace.

Other gems have taken a similar approach:

@@ -213,9 +211,9 @@ def auth(value)
def basic_auth(opts)
user = opts.fetch(:user)
pass = opts.fetch(:pass)
creds = "#{user}:#{pass}"
strict_base64_encoded_creds = ["#{user}:#{pass}"].pack("m0")
Copy link
Member

Choose a reason for hiding this comment

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

We probably can introduce HTTP::Utils (don't like Utils modules in general, but it seems like fitting in this case)withencode_base64method to avoid repeatingpack`. That should make code a bit better.

Copy link
Member

Choose a reason for hiding this comment

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

Or how about HTTP::Encoding, where there are probably a few other things you could stick there to provide a nicer facade for various bits in the standard library

Copy link
Member

Choose a reason for hiding this comment

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

Oh, yeah! HTTP::Encoding is a much better namespace.

Copy link
Author

Choose a reason for hiding this comment

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

While trying to implement this suggestion I stumbled upon a conflict with the build-in Encoding constant. Each usage of the building encoding would need to be prefixed with :: to make ruby look in the global namespace, in addition to required changes in other gems of the ecosystem like http-form-data.

I've went with Encoder instead which doesn't have such issues.

Copy link
Member

Choose a reason for hiding this comment

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

Just saw this comment. Hm, I don't like Encoder name though. Probably we can go with Base64 LOL:

module HTTP
  module Base64
    module_function

    def encode64(input)
      [input].pack("m0")
    end
  end

  class Request
    include Base64

    ...

      digest = encode64("#{proxy[:proxy_username]}:#{proxy[:proxy_password]}")

    ...
  end
end

Copy link
Member

Choose a reason for hiding this comment

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

@tarcieri WDYT?

Copy link
Author

Choose a reason for hiding this comment

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

This does get the tests all passing, however still suffer from the same (though now a bit more theoretical) problem. Other gems that extend the HTTP namespace would now get a different constant when acessing plain Base64.

Luckily the test suite exposed the problem with using Encoding. I can't say if that would be an issue with Base64.

ixti

This comment was marked as outdated.

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.

None yet

3 participants