feat: adds abstraction for resources and subsequent implementation of users. #20

tdstein · 2024-02-15T15:56:52Z

See comments below.

I will come back around and implement the full unit test suite for resources.py and users.py.

github-actions · 2024-02-15T15:58:57Z

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines	Covered	Coverage	Threshold	Status
336	287	85%	0%	🟢

New Files

File	Coverage	Status
src/posit/connect/config.py	100%	🟢
src/posit/connect/config_test.py	100%	🟢
src/posit/connect/endpoints.py	100%	🟢
src/posit/connect/endpoints_test.py	100%	🟢
src/posit/connect/resources.py	51%	🟢
src/posit/connect/resources_test.py	96%	🟢
TOTAL	91%	🟢

Modified Files

File	Coverage	Status
src/posit/connect/init.py	100%	🟢
src/posit/connect/auth.py	100%	🟢
src/posit/connect/auth_test.py	100%	🟢
src/posit/connect/client.py	100%	🟢
src/posit/connect/client_test.py	100%	🟢
src/posit/connect/users.py	62%	🟢
src/posit/connect/users_test.py	100%	🟢
TOTAL	95%	🟢

updated for commit: 6a928cb by action🐍

tdstein · 2024-02-15T16:00:33Z

tinkering.py

+from posit.connect.client import create_client

-client = Client()
-res = client.users.get_current_user()
-print(res.json())
+with create_client() as client:
+    print(client.users.get("f55ca95d-ce52-43ed-b31b-48dc4a07fe13"))
+
+    users = client.users
+    users = users.find(lambda user: user["first_name"].startswith("T"))
+    users = users.find(lambda user: user["last_name"].startswith("S"))
+    user = users.find_one(lambda user: user["user_role"] == "administrator")
+    print(user)


Here is some example usage.

tdstein · 2024-02-15T16:03:15Z

src/posit/connect/resources.py

+from typing import Generic, Iterator, Optional, TypeVar, List, TypedDict, Tuple
+
+
+class Resource(TypedDict):


@nealrichardson - TypedDict seems to be the best of both worlds. We get dict behavior and typed autocomplete, linting, etc...

tdstein · 2024-02-15T16:06:26Z

src/posit/connect/users.py

+    def find_one(self, filter: Callable[[User], bool] = lambda _: True) -> User | None:
+        return next((user for user in self if filter(user)), None)


I'm currently in favor of using functions as arguments for filtering. This allows for more expressive filtering.

This also takes advantage of the lazy-iterable implementation. Once a match is found here, the iterable stops, meaning subsequent fetches to the server are skipped.

I think this is nice, and I like the expressiveness, but it's not compatible with server-side querying/searching, and it also doesn't help the user discover the valid parameter names. It's also annoying to have to write a lambda when I want an exact match (e.g. user.email == me).

So my initial take is +1 for having a filter argument like this, but it shouldn't be the only way.

Ah, yes, thanks for the reminder regarding server-side params.

tdstein · 2024-02-15T16:07:11Z

src/posit/connect/users.py

+        if (index % self.page_size) != 0:
+            #
+            raise ValueError(
+                f"index ({index}) must be a multiple of page size ({self.page_size})"
+            )


Additional logic can be implemented to handle partial fetches, but it's unnecessary since this should only be called at the start of a new page.

tdstein · 2024-02-15T16:13:24Z

src/posit/connect/users.py

+    def get(self, id: str) -> User:
+        user = next((user for user in self if user["guid"] == id), None)
+        if user is None:
+            raise RuntimeError(f"failed to get user with id '{id}'")
+        return user


Notice that there are implementations of get in Users and LazyUsers.

The following implementations therefore have slightly different behavior.

LazyUsers(...).get(...) will call the server.

Users(...).get(...) will NOT call the server.

LazyUsers(...).find(...).get(...) will NOT call the server since .find returns an instance of Users, not LazyUsers.

Generally, an in-memory list iteration will execute faster than a client/server round-trip.

There are other implications that need to be addressed. E.g., what happens when we implement .create?

How does one get LazyUsers or just Users?

The current entry point is client.users:

posit-sdk-py/src/posit/connect/client.py

Line 57 in 6a928cb

self.users: Users = LazyUsers(config=config, session=session)

with create_client(...) as client: users = client.users.find(...).get(...)

Naming is tough, but if I'm inspecting types I get back from the client it might be nice to see something more like Users and CachedUsers.

tdstein · 2024-02-15T16:14:13Z

.github/workflows/pull-request.yaml

+    - if: always()
+      uses: orgoro/[email protected]
+      with:
+          coverageFile: coverage.xml
+          token: ${{ secrets.GITHUB_TOKEN }}


cov will fails < 100%, but show the report anyways in the pull request.

tdstein · 2024-02-15T17:38:13Z

I'm going to merge this to get things on main. Feel free to review it anyway, and I'll follow up.

mmarchetti · 2024-02-16T17:05:46Z

src/posit/connect/config.py

+from typing import Optional
+
+
+def _get_api_key() -> str:


You might consider using NewType to make distinct string types for things that have special meaning (API keys, server URLs, endpoints, ...).

mmarchetti · 2024-02-16T17:10:40Z

src/posit/connect/users.py

-        endpoint = os.path.join(self._endpoint, "__api__/v1/user")
-        return self._session.get(endpoint)
+    def get(self, id: str) -> User:
+        endpoint = os.path.join(self.config.endpoint, "v1/users", id)


I think you'll need urllib.parse.urljoin or posixpath.join, because os.path.join will use os.sep which will be a backslash on Windows.

Thanks. I'll get a safer implementation together! This was quick and dirty :)

See https://github.com/posit-dev/posit-sdk-py/pull/25/files#diff-b7db8bdc96971869d660c47477562a248433cb03cda1f3fa0b423c34105802eaR1

src/posit/connect/endpoints.py

zackverham · 2024-02-16T17:18:02Z

src/posit/connect/endpoints.py

+import os
+import requests
+
+_MAX_PAGE_SIZE = 500


it looks like we aren't using max_page_size as an enforcement tool, but rather as a default value in some places. Wondering if you intend to actually enforce a max page size anywhere - if not, maybe this variable should have a different name?

Ah, yes. Good point. _MAX_PAGE_SIZE means "this is the max page size I can use," but you're right. _PAGE_SIZE plus some ValueError validation would be better.

tdstein added 2 commits February 7, 2024 09:03

feat: adds partial users implementation based on lists

de8d922

feat: adds users implementation with lazy server-side fetching

b5d8a18

tdstein force-pushed the lazy-resources branch from 0785264 to 344e905 Compare February 15, 2024 15:58

feat: adds abstraction for resources.

6a928cb

tdstein force-pushed the lazy-resources branch from 344e905 to 6a928cb Compare February 15, 2024 15:59

tdstein commented Feb 15, 2024

View reviewed changes

tdstein marked this pull request as ready for review February 15, 2024 16:14

tdstein requested review from mmarchetti, nealrichardson, dbkegley, zackverham and jonkeane February 15, 2024 16:14

tdstein merged commit 5d95633 into main Feb 15, 2024
6 of 7 checks passed

mmarchetti reviewed Feb 16, 2024

View reviewed changes

zackverham reviewed Feb 16, 2024

View reviewed changes

src/posit/connect/endpoints.py Show resolved Hide resolved

zackverham reviewed Feb 16, 2024

View reviewed changes

nealrichardson deleted the lazy-resources branch February 16, 2024 19:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: adds abstraction for resources and subsequent implementation of users. #20

feat: adds abstraction for resources and subsequent implementation of users. #20

tdstein commented Feb 15, 2024 •

edited

Loading

github-actions bot commented Feb 15, 2024 •

edited

Loading

tdstein Feb 15, 2024

tdstein Feb 15, 2024

tdstein Feb 15, 2024

nealrichardson Feb 15, 2024

tdstein Feb 15, 2024

tdstein Feb 15, 2024

tdstein Feb 15, 2024

nealrichardson Feb 15, 2024

tdstein Feb 15, 2024

mmarchetti Feb 16, 2024

tdstein Feb 15, 2024

tdstein commented Feb 15, 2024

mmarchetti Feb 16, 2024

mmarchetti Feb 16, 2024

tdstein Feb 16, 2024

tdstein Feb 16, 2024

zackverham Feb 16, 2024

tdstein Feb 16, 2024

		from typing import Generic, Iterator, Optional, TypeVar, List, TypedDict, Tuple


		class Resource(TypedDict):

		def find_one(self, filter: Callable[[User], bool] = lambda _: True) -> User \| None:
		return next((user for user in self if filter(user)), None)

feat: adds abstraction for resources and subsequent implementation of users. #20

feat: adds abstraction for resources and subsequent implementation of users. #20

Conversation

tdstein commented Feb 15, 2024 • edited Loading

github-actions bot commented Feb 15, 2024 • edited Loading

☂️ Python Coverage

Overall Coverage

New Files

Modified Files

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdstein commented Feb 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdstein commented Feb 15, 2024 •

edited

Loading

github-actions bot commented Feb 15, 2024 •

edited

Loading