Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

brooklynbagel
Copy link

Addresses the owner property of ContentItem as discussed in #165

I ran all the steps required plus a manual test with the following output:

from posit.connect import Client

with Client() as client:
  all_content = client.content.find()
  print(all_content[0].owner)
# {'guid': '867f7ef5-6cb4-4a4d-908d-21f7ccacf273', 'username': 'michael.beigelmacher@posit.co', 'first_name': 'Michael', 'last_name': 'Beigelmacher'}

@brooklynbagel brooklynbagel force-pushed the brooklynbagel/fix-content-item-user-type branch from a9cb2f1 to 467f715 Compare May 3, 2024 02:24
@brooklynbagel brooklynbagel changed the title add dataclass to represent ContentItem owner feat: add dataclass to represent ContentItem owner May 3, 2024
Copy link
Collaborator

@tdstein tdstein left a 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.

@brooklynbagel brooklynbagel force-pushed the brooklynbagel/fix-content-item-user-type branch from 467f715 to 8e2e479 Compare May 3, 2024 18:32
@brooklynbagel
Copy link
Author

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.

Interesting, this is something I'll look. Thanks for the note and I wasn't sure because there were a couple of places where @dataclass are used. See

@dataclass
class Page:
"""
Represents a page of results returned by the paginator.
Attributes
----------
current_page (int): The current page number.
total (int): The total number of results.
results (List[dict]): The list of results on the current page.
"""
current_page: int
total: int
results: List[dict]
and
@dataclass
class CursorPage:
paging: dict
results: List[dict]

@brooklynbagel
Copy link
Author

brooklynbagel commented May 3, 2024

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

Run orgoro/coverage@v3.1
  with:
    coverageFile: coverage.xml
    thresholdAll: 0.8
    token: ***
    thresholdNew: 0
    thresholdModified: 0
  env:
    pythonLocation: /opt/hostedtoolcache/Python/3.12.3/x64
    PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.12.3/x64/lib/pkgconfig
    Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.3/x64
    Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.3/x64
    Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.3/x64
    LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.12.3/x64/lib
comparing commits: base 62ff3596c56217d3f5fb2d57d95bde53f5655b4f <> head 8e2e479cfd64d350739c84eade90abe87bf43831
git new files: [] modified files: ["src/posit/connect/content.py"]
source: /home/runner/work/posit-sdk-py/posit-sdk-py
Results
  Average coverage 96% ✅
  No covered new files in this PR 
  Modified files coverage ✅
HttpError: Resource not accessible by integration
    at /home/runner/work/_actions/orgoro/coverage/v3.1/webpack:/typescript-action/node_modules/@octokit/request/dist-node/index.js:86:1
    at processTicksAndRejections (node:internal/process/task_queues:96:5)

@brooklynbagel brooklynbagel requested a review from tdstein May 3, 2024 18:40
@tdstein
Copy link
Collaborator

tdstein commented May 3, 2024

Thanks for the note and I wasn't sure because there were a couple of places where @dataclass are used.

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!

@tdstein
Copy link
Collaborator

tdstein commented May 3, 2024

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.

Yes, we can ignore that. We have an issue to resolve that. It's a limitation of forks versus branches in GitHub Actions.

@brooklynbagel
Copy link
Author

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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants