-
Notifications
You must be signed in to change notification settings - Fork 2
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
Patch Api v1 #1
Patch Api v1 #1
Conversation
1dd962f
to
e538a90
Compare
66c82cf
to
f7679cf
Compare
@@ -0,0 +1,5 @@ | |||
repos: |
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.
Configuration for using the pre-commit package and black code formatter.
@@ -32,27 +32,24 @@ pip install patch | |||
|
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.
Read through the README to get an idea of the interface!
README.md
Outdated
``` | ||
|
||
The `api_client` will be used to instantiate other API objects for Patch resources, for example the `OrdersApi`: | ||
|
||
``` | ||
import patch_api | ||
from patch_api.api.orders_api import OrdersApi | ||
from patch_api.api.orders_api import OrdersApi as Orders |
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.
We talked about dropping the {Resource}Api
from the API models and I tried to implement it but couldn't figure out how to do it well, so specifying to use aliased imports was a workaround to get a cleaner interface. LMK what you think.
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.
hmm maybe we can import these resources on __init__.py
but rename them, and then expose/make them public by using __all__
. 🤔
Not sure if it would work, would need some testing.
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 tried doing that in code in a template in client-code-generation, but the variables weren't being evaluated properly? I can try again though, it feels like it should be possible. Worst case scenario we can have a template that we have to manually update when new API resources are being added.
README.md
Outdated
configuration = patch_api.Configuration(api_key=os.environ.get('SANDBOX_API_KEY')) | ||
api_client = patch_api.ApiClient(configuration) | ||
orders_api = OrdersApi(api_client=api_client) | ||
api_client = patch_api.ApiClient(api_key=os.environ.get('SANDBOX_API_KEY')) |
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 api_client
instance can be reused, but each resource interface instance (Orders, Estimates, etc) need to be instantiated with an api_client
instance.
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.
is there a way to pass the api_key to the api resources?
Maybe as public configuration variable?
We could do something like:
patch_api.api_key = "blah"
This could live on __init__.py
.
See this example: https://github.com/stripe/stripe-python/blob/master/stripe/__init__.py
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.
Yes.... that is much better and we don't have to init/pass around api_client objects all over the place. Will look at this today!
eb8941e
to
da99e9c
Compare
da99e9c
to
7fbf49f
Compare
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.
nice job, thanks for working on this!
I left a couple of comments and suggestions, let me know what you think!
README.md
Outdated
configuration = patch_api.Configuration(api_key=os.environ.get('SANDBOX_API_KEY')) | ||
api_client = patch_api.ApiClient(configuration) | ||
orders_api = OrdersApi(api_client=api_client) | ||
api_client = patch_api.ApiClient(api_key=os.environ.get('SANDBOX_API_KEY')) |
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.
is there a way to pass the api_key to the api resources?
Maybe as public configuration variable?
We could do something like:
patch_api.api_key = "blah"
This could live on __init__.py
.
See this example: https://github.com/stripe/stripe-python/blob/master/stripe/__init__.py
README.md
Outdated
from patch_api.api.orders_api import OrdersApi as Orders | ||
|
||
api_client = patch_api.ApiClient(api_key=os.environ.get('SANDBOX_API_KEY')) | ||
orders_api = Orders(api_client=api_client) |
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 think one thing that could be done here is to implement the __getattr__
method and use it to chain the calls to the api resources. It works pretty much like ruby's method_missing
.
I'm pretty sure it would allow us to do something like:
patch_api.orders.create_order(some_attrs)
By implementing __get_attr__
, you could return a new instance of the requested ApiResource (in this case, OrdersApi) and pass in self.api_client
or the self.api_key
from the parent object.
This could make this interface a little cleaner.
Here's an example from the chargify python api: https://github.com/ipmb/chargify-python/blob/ed6e49559da53f36006800bc3355d31696b01175/chargify.py#L110
Here's an explanation of how it works: https://blog.rmotr.com/python-magic-methods-and-getattr-75cf896b3f88
Let me know if that makes sense!
89147aa
to
e58c0b9
Compare
…python into lovisa/python-v1
1171e3a
to
24c8c23
Compare
24c8c23
to
fe8831e
Compare
3884675
to
e9d14fa
Compare
e9d14fa
to
ca9a692
Compare
4f3539f
to
e1ff181
Compare
e1ff181
to
456ba16
Compare
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.
looks good to me, but left a few comments.
Co-authored-by: Thiago Araujo <thd.araujo@gmail.com>
01544e4
to
0716e42
Compare
What
Adding v1 of the package
Why
To make our Python using customers happy
SDK Release Checklist
Outstanding work