Skip to content

Conversation

@straight-shoota
Copy link
Member

This overload is a no-op, so I don't see how it would be useful for anything. It avoids a nil check at the call site, but adding a nil is makes no sense conceptually.

@lasso
Copy link
Contributor

lasso commented Nov 3, 2025

I don't quite understand why the OAuth::Params struct doesn't rely on the URI::Params struct instead. Wouldn't that simplify things (in all cases except to_s?

# src/oauth/params.cr

# :nodoc:
struct OAuth::Params
  def initialize
    @params = URI::Params.new
  end

  def add(key : String, value : String?) : Nil
    @params.add key, value unless value.nil?
  end

  def add_query(query : String) : Nil
    @params.add_query query
  end

  def to_s(io : IO) : Nil
    sorted = @params.to_a.sort_by &.first
    sorted.each_with_index do |(key, value), i|
      io << "%26" if i > 0
      URI.encode_www_form key, io, space_to_plus: false
      io << "%3D"
      URI.encode_www_form value, io, space_to_plus: false
    end
  end
end

@straight-shoota
Copy link
Member Author

straight-shoota commented Nov 3, 2025

That's a good observation.

Might have to do with the fact that OAuth::Params is actually older than URI::Params (including its pre-predecessor name HTTP::Form). 🤯

Actually, it looks like OAuth::Params might be entirely disposable. It's an internal type, only used in two private methods in OAuth::Signature (which itself is a private type).
We can trivially implement its functionality with just URI::Params and a custom serialization method.

So I think we should probably deprecate (or directly remove?) OAuth::Params entirely instead of this change.

@ysbaddaden
Copy link
Contributor

It's undocumented, so I guess we can just drop it 👍

@straight-shoota
Copy link
Member Author

Closing in favour of #16317

@straight-shoota straight-shoota deleted the refactor/oauth-signature branch November 3, 2025 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants