feat: adds groups #227
feat: adds groups #227
Conversation
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
tdstein
force-pushed
the
tdstein/find-groups
branch
from
d2c53a6 to
9b11d5c
Compare
| path = "v1/groups" | ||
| url = urls.append(self.config.url, path) |
There was a problem hiding this comment.
Any reason to have path as a separate variable?
There was a problem hiding this comment.
not here, but comes into play downstream: https://github.com/posit-dev/posit-sdk-py/pull/228/files#diff-b7d2a2eb92d17a5b4aff237de4af47283494386561a1e96fea70f0d5ba4201d8R207-R211
There was a problem hiding this comment.
This is looking good, one minor comment. (Sorry I didn't use the actual review flow with that one, wasn't sure how long I would be able to pay attention to this and didn't want comments dangling)
Unit tests are sparse since we have integration tests now.
The test you have there is probably good enough, but IMHO and generally: leaning too heavily on integration tests will make development much harder. Having robust unit tests means it's quick + easy to get started and poke at it without relying on throwing it at CI and waiting for that to churn.
💯 Completely agree. I'm hoping to get more unit tests in as I fill out the rest of the group's API. |
| class TestGroups: | ||
| def setup_class(cls): | ||
| cls.client = connect.Client() | ||
| cls.item = cls.client.groups.create(name="Friends") | ||
|
|
||
| def teardown_class(cls): | ||
| cls.item.delete() | ||
| assert cls.client.groups.count() == 0 | ||
|
|
||
| def test_count(self): | ||
| assert self.client.groups.count() == 1 | ||
|
|
||
| def test_get(self): | ||
| assert self.client.groups.get(self.item.guid) | ||
|
|
||
| def test_find(self): | ||
| assert self.client.groups.find() == [self.item] | ||
|
|
||
| def test_find_one(self): | ||
| assert self.client.groups.find_one() == self.item |
| @@ -94,6 +95,17 @@ def oauth(self) -> OAuthIntegration: | |||
| """ | |||
| return OAuthIntegration(config=self.config, session=self.session) | |||
|
|
|||
| @property | |||
| def groups(self) -> Groups: | |||
| """The groups resource interface. | |||
There was a problem hiding this comment.
My gut response is that I'd want to see more descriptive text here, if this is used as doc text in the CLI. I don't know if "groups resource interface" will be a particularly meaningful term for our users (it isn't for me).
I see that some of this (resource) is terminology used throughout the package, and maybe it'll make more sense to me as I get to know it.
There was a problem hiding this comment.
Good call out. The entire Client class could use some documentation love.
Unit tests are sparse since we have integration tests now.