Own-and-Ship / oas_agent

The Own & Ship Ruby agent
2 stars 0 forks source link

Replace keyword args in a sustainable fashion #27

Closed caius closed 9 months ago

caius commented 10 months ago

As part of #25 we've had to remove keyword arguments from the codebase, so we've gone from

def push(message:, non_block: true)
  message
  non_block
end

to

def push(options = {})
  message = options.fetch(:message)
  non_block = options.fetch(:non_block, true)
end

which is only really discoverable when you start reading the source code, and it's only convention that we're pulling out the variables at the start of the method which keeps them easily findable.

We could move to positional arguments, this would work well with IDE tooling to prompt us with helper tooltips when calling other methods. Loses the nicety of the call-site, but also matches mostly what we did back in the Dark-ish ages of Ruby 1.9.3.

We could instead add YARD documentation for each of these methods, defining the arguments expected in the options hash. Gives us one more place to define what's needed in the argument list.

The options = {} pattern likely requires test coverage to be happy we've caught all the required cases, to stop us leaking nil, FetchError or NoMethodError into the world. Highly tempted to drop all the nicety and go back to positional arguments:

def push(message, non_block = true)
  message
  non_block
end

What are your thoughts @wjessop?

wjessop commented 10 months ago

I think positional arguments with a guard for nil values/default values would be more discoverable/idiomatic. YARD docs would be welcome and something we should probably standardise on, though it makes me sad for the lack of static typing in Ruby ;)

caius commented 10 months ago

I wonder if we could add typing (RBS is separate files right?) whilst maintaining backwards compatibility 🤔

Positional arguments and YARD docs make sense to me 👍🏻

wjessop commented 10 months ago

RBS would be a decent addition.