-
Notifications
You must be signed in to change notification settings - Fork 81
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
Allow implicit options to be passed into product.sku #84
base: master
Are you sure you want to change the base?
Conversation
- Pass through api_path object to connection - Append catalog word to resource url - Ignore meta field when returning response
Not sure if this project is being actively maintained anymore? I also just added a quick and easy approach to adding v3 support (lacking some structured testing) |
Example call with v3 apiv3 = bigcommerce.api.BigcommerceApi(
client_id=client_id,
store_hash=store_hash,
access_token=access_token,
api_path='/stores/{}/v3/{}'
)
apiv3.Variants.all() |
@karen-white Seems to work, any chance for implementation? We would also like to add two other key value pairs on the order, but you don't seem to accept merge requests. |
return ProductSkus.all(self.id, connection=self._connection) | ||
return ProductSkus.all(self.id, connection=self._connection, **kwargs) | ||
|
||
def variants(self, id=None, **kwargs): |
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.
I would prefer to have the V3 products-related endpoints in a completely separate file from the V2 ones - that namespacing will help a lot if we try to apply different logic to them (pagination comes to mind)
This looks good, but I'd like to draw a firmer line between V2 and V3 APIs within the library as those APIs have pretty different schemas and behaviors. |
@bookernath any idea when this may get merged in? it looks like a great way to get cracking with some of the v3 functionality. I'm starting to hit issues needing some of the newer v3 stuff. I don't really want to make an API client from scratch when you have this great library :) |
@Bobspadger I hear you - I'd love to unblock V3 usage via this library as well. I just want to make sure we're set up for success adding additional V3-specific APIs and behavior in the future, hence my above comments. Happy to work with @VDuda to see this over the line. |
@bookernath cool. I'll keep testing it over the following weeks as I build out our store using the api. |
@bookernath could you add some more detail about how you would like the namespaces broken up? As you have to connect to a different endpoint when you create your connection, how would you anticipate the logic of organising this so it is v2 and v3 compatible ? I'm happy to start working on some of it, but would like a firmer direction before starting :) |
@bookernath It would be really great to get a direction considering @VDuda 's enthusiasm for V3 and the fact there hasn't been much movement here in 2018 and 2017. |
@@ -53,6 +53,10 @@ def _run_method(self, method, url, data=None, query=None, headers=None): | |||
if headers is None: | |||
headers = {} | |||
|
|||
# Support v3 |
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.
@VDuda with this bit, the orders API no longer works, as this seems to have an explicit /v2/
path as per the docs here:
https://developer.bigcommerce.com/api-reference/orders/orders-api/orders/getallorders
This bit of the code injects catalog
in when it is not needed, the url for orders is /store/{store_hash}/v2/orders
Using the code this way would require making a fresh connection for a v2
connection so we don't break the rest of the API
@bookernath am I missing anything with your API endpoints here?
What?
Essentially we'd like to completely utilize the functionality already available to us when we call product.skus() - the underlying all() function allows us to pass product options, but we're limited within the product.skus() function to simply pass an id object. By allowing kwargs into product.skus() we can take advantage of the additional optionality within the all function.
We could then do something neat like this. Implicitly specifying which pages to query. And I suspect we could also utilize the filter options as well - but I need to look into that.
Tickets / Documentation