Move signature logic to its own module

[EmilioCristalli]

Not sure if this change was planned, but seemed aligned with the rest of modules the gem has.
Also I think this will help to start working on redefining the public API (I guess that is planned for version 2).

Let me know what you think, if you want me to make changes, or if it's just not the direction you guys wanted to go.

Thanks!

[excpt]

Thank you very much!

This is exactly the way to go. 👍

[excpt]

Please use module_function instead of extend self

[EmilioCristalli]

what about private functions? I don't think it's possible to define private function with module_functions.
What are the benefits of module_functions?

[excpt]

Using module_function is the preferred coding style.

You're alway able to define a function as private by using this syntax:

def my_fun(my_param) do
  puts my_param
end
private :my_fun

[EmilioCristalli]

I don't think module_function respects the private visibility.

module A
  module_function

  def my_fun(my_param)
    puts my_param
  end
  private :my_fun
end

A.my_fun("calling private method")   #=>  "calling private method"

[excpt]

Please merge the current master into your branch. There have been changes. The small issues are commented inline.

[EmilioCristalli]

Thanks for the comments @excpt

This branch is already up-to-date with master, sorry I missed NAMED_CURVES was moved to the DefaultOptions module.

In my opinion the constants *_ALGORITHMS and NAMED_CURVES make more sense in the Signature module. This module is the only one that uses them, and they don't seem to be configurable like the other options, so I would rather remove NAMED_CURVES from the DefaultOptions module.

Do you still think it's worth moving the constants to DefaultOptions?

[excpt]

Agreed. Move the algorithms into the signature module. This makes way more sense. The rest is looking fine. After that change i'll merge your PR.

Thanks again. 🍻

[EmilioCristalli]

@excpt I just removed NAMED_CURVES in this commit 8ed32d7.

Let me know what you want to do with the module_function vs extend self discussion

[excpt]

After looking through the last refactoring to move logic out of the jwt.rb monolith I think the better solution to that problem is to rewrite the module as a class. The Encoding and Verify refactorings took the same approach. This will give you full control about what's private logic and what should be exposed to the outside.

[EmilioCristalli]

I think normally a class is meant to have both behavior and state. In this case, Signature doesn't really need state, it's more like two util methods that would let you generate or verify a signature (given an algorithm and key).

So, if you implement Signature as a class you would either use instance methods and you would have to create a new instance to use the methods (which shouldn't be necessary because it doesn't have any state) or you use class methods (with private class methods too), which basically is the same as the module with extend self, but it feels like a module better defines the purpose of the util functions, because it doesn't hold state.

And I think module with extend self gives you as much control of private methods as a class (actually I think to define private class methods is a little trickier because you have to either use class << self or private_class_method)

@excpt do you still prefer to implement this as a class? would you use instance or class methods?

[excpt]

I totally over-engineered the problem. Sorry.

Thank you very much for the contribution and your patience to withstand my pretty silly review. 👍

The code solves the problem and this is what counts.

[excpt]

@EmilioCristalli
Please take a look at your PR #196 contribution. After merging this PR your new refactor PR has now merge conflicts.

[EmilioCristalli]

no problem @excpt !

I just rebased and solved conflicts on #196