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

Make 'timestamp' a positional argument #34

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pelted
Copy link

@pelted pelted commented Aug 16, 2023

#generate() uses positional arguments for everything but the last timestamp keyword argument. This change is more for consistency.

Mixing positional and keyword arguments is normal. This method already has 4 existing positional arguments in front of this one which forces callers to reason more about how to use this method when the last argument is suddenly a keyword. 1

Footnotes

  1. Another, arguably more expandable, approach would be a **options splat and pass timestamp in a hash.

@@ -6,7 +6,7 @@

module Krypto
class Challenge
def self.generate(signing_key, challenge_id, challenge_data, request_data, timestamp: Time.now)
def self.generate(signing_key, challenge_id, challenge_data, request_data, timestamp = Time.now)
Copy link
Contributor

Choose a reason for hiding this comment

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

Idiomatically, I'm thinking about this as having 4 required arguments, and then 1 optional timestamp. Potentially, in the future, there may be more optional arguments. Knowing that, do you want to move timestamp to positional?

Copy link
Author

@pelted pelted Aug 16, 2023

Choose a reason for hiding this comment

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

Knowing that I would expect the method to look something like this

def self.generate(signing_key, challenge_id, challenge_data, request_data, **options)
   options = { timestamp: Time.now.utc.to_i }.merge(options)
   ...
end

or

def self.generate(signing_key:, challenge_id:, challenge_data:, request_data:, **options)
   options = { timestamp: Time.now.utc.to_i }.merge(options)
   ...
end

but I don't think this 2nd one is Ruby 1.9 compatible.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we need ruby 1.9.

I think timestamp: Time.now gives us a better path to get to **options.

Thinking about it, I'm uncomfortable with optional positional arguments. I'd suggest moving to all named style, or leaving as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants