-
Notifications
You must be signed in to change notification settings - Fork 137
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
Add pagination and tests for Client Grants #116
Conversation
lib/auth0/api/v2/client_grants.rb
Outdated
def client_grants (page: nil, per_page: nil) | ||
request_params = { | ||
page: !page.nil? ? page.to_i : nil, | ||
per_page: !page.nil? && !per_page.nil? ? per_page.to_i : nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to simplify this down to page.nil? || per_page.nil? ? nil : per_page.to_i
. There are a lot of symbols going on already, so reducing the cognitive overhead of negated AND
s should help reduce the complexity.
I'll also add that if anyone drops in a boolean to these params, the method will raise since to_i
is not implemented on booleans. I'm assuming this logic exists just to cast strings to ints though. And some of the logic seems confusing to read through. Here is a potential alternative to the method that I'm not sure I like any better, but 🤷♂️.
def client_grants(page: nil, per_page: nil)
return get(client_grants_path) if page.nil?
clean_per_page = per_page.nil? ? nil : per_page.to_i
get(client_grants_path, page: page.to_i, per_page: clean_per_page)
end
It explicitly shows that if page
is nil
we have nothing to do here, then continues on with the logic.
Ideally you could get rid of the last ternary if the logic of passing per_page
supports it, but that's something you'd have to check out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good feedback, thank you. I'll see if I can simplify this a little better in a new commit.
@machuga - Removed casting here since it was not doing anything (string values passed were still returning the correct results) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
page
andper_page
parameters toAuth0::Api::V2::ClientGrants::client_grants
endpointsleep
directives from the client grant integration tests