Design by wishful thinking for Connect SDK #16
Conversation
Co-authored-by: Neal Richardson <[email protected]>
|
|
||
| ``` | ||
| my_app.update(title="Quarterly Analysis of Team Velocity") | ||
| my_app.permissions.find_one(email="[email protected]").update(role="owner") |
There was a problem hiding this comment.
I do like this quite a bit. +1 for find_one.
There was a problem hiding this comment.
Assuming that find_one(kwargs) is find(kwargs)[0], should it error if find() returns len != 1?
There was a problem hiding this comment.
I've associated find with an Optional return value in other languages. (e.g., go find this thing if you can; otherwise, let me know you came up empty-handed.)
There was a problem hiding this comment.
I restored .find_one() and more explicitly described behavior, LMK if that's what you had in mind.
| ``` | ||
| from posit.connect import Client | ||
|
|
||
| con = Client() | ||
| ``` |
There was a problem hiding this comment.
| ``` | |
| from posit.connect import Client | |
| con = Client() | |
| ``` | |
| from posit.connect import create_client | |
| with create_client() as client: | |
| client... |
This helps with resource management.
@contextmanager
def create_client(
api_key: Optional[str] = None, endpoint: Optional[str] = None
) -> Generator[Client, None, None]:
"""Creates a new :class:`Client` instance
Keyword Arguments:
api_key -- an api_key for authentication (default: {None})
endpoint -- a base api endpoint (url) (default: {None})
Returns:
A :class:`Client` instance
"""
client = Client(api_key=api_key, endpoint=endpoint)
try:
yield client
finally:
del clientThere was a problem hiding this comment.
What resource do you expect client to be holding onto?
There was a problem hiding this comment.
Currently, it holds a requests.Session object, which has a close method.
There was a problem hiding this comment.
Is there anything that needs to be cleaned up that won't be handled on exit?
There was a problem hiding this comment.
Just the requests.Session instance that needs to be released. Otherwise, the GC should handle most scenarios.
| Entities have methods that are appropriate to them. Fields in the entity bodies can be accessed as properties. | ||
|
|
||
| ``` | ||
| for st in con.content.find(app_mode="streamlit"): |
There was a problem hiding this comment.
Regarding the .find(app_mode="streamline") vs .find({ "app_mode": "streamline"}). I'm unsure what it looks like to support API differences between Connect versions.
There was a problem hiding this comment.
There are many ways to address this and we don't need to solve it now. But, I would like to note it.
There was a problem hiding this comment.
IMO we make the Python interface whatever we think it should be, and internally map parameters to whatever the connect api/version uses.
There was a problem hiding this comment.
I added some language about this below, LMK what you think.
| * If no entities match the query, `.find()` returns a length-0 collection. | ||
| * Iterating over a collection without having first called `find()` is equivalent to having queried for all. | ||
| * `find()` should use query-based REST APIs where existing, and fall back to retrieving all and filtering client-side where those APIs do not (yet) exist. | ||
| * Should `collection.find().find()` work? Probably. |
There was a problem hiding this comment.
Yes, and the second call should operate on in-memory/cached results.
Just to start discussion
Fixes #9