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

API v2 #27

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

API v2 #27

wants to merge 7 commits into from

Conversation

maximeCony
Copy link

@maximeCony maximeCony commented Apr 27, 2020

Update Ruby lib to APIv2.

The generation code lives in https://github.com/Patreon/patreon_py/pull/18680

Note to reviewer

It should be easier to look at each commit individually.

Inspiration: Patreon/patreon-python#28

@maximeCony maximeCony force-pushed the mcony-BECORE-112-ruby-api-v2 branch from a09a8d4 to d79f44f Compare April 29, 2020 23:13
@maximeCony maximeCony force-pushed the mcony-BECORE-112-ruby-api-v2 branch from d79f44f to e08c827 Compare April 30, 2020 18:51
@maximeCony maximeCony requested review from MrRoboman and kourge April 30, 2020 21:15
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
lib/patreon/api.rb Outdated Show resolved Hide resolved
parsed_url = URI.parse(url)
params = parsed_url.query ? Rack::Utils.parse_query(parsed_url.query) : {}
params['include'] = joined_or_null(includes) if includes
fields.each do |name, val|
params["fields[#{name}]"] = val
end if fields
params["page[count]"] = (count || 10) unless count_not_passed
Copy link

Choose a reason for hiding this comment

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

This desugars to:

if not count_not_passed
  params["page[count]"] = (count || 10)
end

When this method is:

  • Called with count set to 3, then count_not_passed == false, count == 3. This sets params["page[count]"] to 3 || 10, which evaluates to 3.
  • Called with count set to nil, then count_not_passed == false, count == nil. This sets params["page[count]"] to nil || 10, which evaluates to 10.
  • Called without count, then count_not_passed == true, count == nil. This doesn't set params["page[count]"] at all.

It looks like the intent is to provide three choices?

  1. Use the count specified.
  2. Use default count.
  3. Omit count.

If this triple-choice is to stay, I recommend turning the rest of the optional arguments into keyword args. Depending on the minimum Ruby version supported, we could either use real Ruby keyword args, or simulate them with a hash, as is a long-time traditional in older versions of Ruby.

Copy link
Author

Choose a reason for hiding this comment

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

Great catch and great explanation! Looking at the python lib I don't even think we need to provide a default. I will update the logic to:

  1. Use the count specified.
  2. Omit count.

@kourge
Copy link

kourge commented May 1, 2020

Although this Enum utility comes from an external source, I'm also not convinced that const_missing is the right way to implement enums.

The instance-variant of const_missing, which is the much more popular method_missing, has a companion method called Object#respond_to_missing?. It's meant to act as a fallback for respond_to?. There's no exact const-variant of respond_to_missing?, but one way to work around it is to turn this model inside out: instead of using const_missing to return a value for a constant that does not otherwise exist, use const_set to define a constant once. For enumerating all constants — something that methods like each, all, all_to_hash rely on — either the built-in Module#constants can serve that purpose, or, to keep them separate from other non-code-gen'd constants, we can maintain an array of @@enum_constants.

@maximeCony
Copy link
Author

Although this Enum utility comes from an external source, I'm also not convinced that const_missing is the right way to implement enums.

Do you think it is worth creating a separate PR to address that? Also wondering if there is a ruby gem that we could want to use.

@kourge
Copy link

kourge commented May 1, 2020

Yes, the author of that snippet has created a separate ruby-enum gem that does this right.

Edit: Yes, this can be addressed in another PR.

@kourge
Copy link

kourge commented May 1, 2020

I assume all changes to README.md here will also be ported over to its template source?

@maximeCony
Copy link
Author

I assume all changes to README.md here will also be ported over to its template source?

Yep! (Thanks for the reminder 😉)

@maximeCony maximeCony force-pushed the mcony-BECORE-112-ruby-api-v2 branch from 3009ed2 to ec467c7 Compare May 1, 2020 22:45
@maximeCony maximeCony marked this pull request as ready for review May 4, 2020 22:03
lib/patreon/utils/jsonapi/url_util.rb Outdated Show resolved Hide resolved
Co-authored-by: Wil Lee <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants