workos-ruby icon indicating copy to clipboard operation
workos-ruby copied to clipboard

Make events parameter required for list_events

Open mattgd opened this issue 1 year ago • 5 comments

Description

Make events parameter required for list_events.

Documentation

Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.

[ ] Yes

If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.

mattgd avatar Apr 11 '24 15:04 mattgd

I'm not much of a rubyist. Do I need to update the method signature or comment at all on this options hash to reflect this parameter requirement? https://github.com/workos/workos-ruby/blob/2a90a0bee142c31b11cfe3d85017f5d505f3f0ec/lib/workos/events.rb#L16-L24

mattgd avatar Apr 11 '24 15:04 mattgd

This change doesn't look to be doing what you want. The InvalidRequestError would come from making the request with no parameters, not the SDK telling the user they need to add a parameter. If event is a required parameter I would probably change the signature of list_event to thus:

def list_event(event:, options = {})
end

This way the user will get a ArgumentError when they forget event and this should also be backwards compatible to current callers to this function.

jasonroelofs avatar Apr 11 '24 16:04 jasonroelofs

Ah, right, that syntax isn't 2.7 compatible. You might need to add extra logic looking for options[:event] and throw an ArgumentError if it doesn't exist.

jasonroelofs avatar Apr 24 '24 14:04 jasonroelofs

Ah, right, that syntax isn't 2.7 compatible. You might need to add extra logic looking for options[:event] and throw an ArgumentError if it doesn't exist.

Ah okay. Let me move make some changes.

mattgd avatar Apr 24 '24 14:04 mattgd