-
Notifications
You must be signed in to change notification settings - Fork 467
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 api versions for url prefix control #548
Conversation
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.
Looking great. A few comments so far.
@@ -65,8 +66,9 @@ def encoded_params_for_signature(params) | |||
end | |||
end | |||
|
|||
def initialize(url, token = nil, extra = {}) | |||
def initialize(url, token = nil, api_version = :no_version, extra = {}) |
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.
Could api_version
be a kwarg? Same q for temp
.
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.
compatibility is my biggest concern with that.
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.
But couldn't a kwarg just have a default, too?
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.
could you provide the full signature you are picturing?
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.
(See Slack)
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.
Converged on "maybe it is better to just go to keyword args", i.e. def initialize(url:, token:, api_version:, ...)
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.
The method signature will change in the next PR, in which api_version
becomes required.
eeddd7a
to
730eae1
Compare
730eae1
to
98cd7ba
Compare
98cd7ba
to
333787d
Compare
333787d
to
1887de8
Compare
Add api version to control the url prefix of both the graphql schema and rest resources.
Coercion support from strings and symbols of all the defined api versions.
The version has been added to the ShopifyAPI::Base as a thread safe attribute when that is available.
It is also added to the session and defaulted to
no_version
to match current behaviour.Session usage looks like:
ShopifyAPI::Session.new("https://this-is-my-test-shop.myshopify.com", "test_token", :no_version)
ShopifyAPI::Session.new("https://this-is-my-test-shop.myshopify.com", "test_token", 'unstable')