-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: add dataclass to represent ContentItem owner #190
feat: add dataclass to represent ContentItem owner #190
Conversation
a9cb2f1
to
467f715
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.
Thanks for the pull request @brooklynbagel. We have taken a different approach to modeling schemas in this project. Instead of using a @dataclass
please modify the implementation to inherit from resources.Resource
and create @property
attributes for each field. You can mimic the ContentItem
.
If you are interested in learning more about this approach, there are various discussions in the issue history. But, the basic idea is that this pattern provides an escape hatch for forward and backwards compatibility between server (Connect) versions. It also provides an intuitive experience when converting collections of these datatypes to Pandas and Polars data frames.
467f715
to
8e2e479
Compare
Interesting, this is something I'll look. Thanks for the note and I wasn't sure because there were a couple of places where posit-sdk-py/src/posit/connect/paginator.py Lines 11 to 25 in 19ec089
posit-sdk-py/src/posit/connect/cursors.py Lines 12 to 15 in 19ec089
|
I can mark this as ready for review but I'm not sure why CI is failing for Python 3.12 and if it's an error we can ignore. @tdstein
|
Ah, since those are internal, I used dataclasses. However, on second thought, it would be better to use a resource type since the API response schema can change between Connect versions. Thanks for calling those out! |
Yes, we can ignore that. We have an issue to resolve that. It's a limitation of forks versus branches in GitHub Actions. |
Got it, I'll keep that in mind if I submit any future PRs. I don't have write access so I cannot perform the merge myself. |
Addresses the owner property of ContentItem as discussed in #165
I ran all the steps required plus a manual test with the following output: