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 events parameter required for list_events #282

Conversation

mattgd
Copy link
Contributor

@mattgd mattgd commented Apr 11, 2024

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 mattgd self-assigned this Apr 11, 2024
Copy link

linear bot commented Apr 11, 2024

@mattgd mattgd marked this pull request as ready for review April 11, 2024 15:53
@mattgd
Copy link
Contributor Author

mattgd commented Apr 11, 2024

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?

# @param [Hash] options An options hash
# @option options [String] event The type of event
# retrieved.
# @option options [String] limit Maximum number of records to return.
# @option options [String] after Pagination cursor to receive records
# after a provided Event ID.
#
# @return [Hash]
def list_events(options = {})

@jasonroelofs
Copy link
Contributor

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
Copy link
Contributor

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.

@mattgd
Copy link
Contributor Author

mattgd commented Apr 24, 2024

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 mattgd added the breaking change Contains a breaking change label May 29, 2024
@mattgd mattgd merged commit a2cabff into main Jun 6, 2024
4 checks passed
@mattgd mattgd deleted the feature/dsync-1309-update-ruby-sdk-to-require-event-names-parameter branch June 6, 2024 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Contains a breaking change
Development

Successfully merging this pull request may close these issues.

3 participants