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

fix: add fetch to packages protocol #359

Merged
merged 1 commit into from
Dec 16, 2024
Merged

Conversation

tdstein
Copy link
Collaborator

@tdstein tdstein commented Dec 13, 2024

No description provided.

Copy link

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
1948 1790 92% 0% 🟢

New Files

No new covered files...

Modified Files

File Coverage Status
src/posit/connect/client.py 99% 🟢
src/posit/connect/content.py 96% 🟢
src/posit/connect/packages.py 0% 🟢
TOTAL 65% 🟢

updated for commit: 9274727 by action🐍

@tdstein
Copy link
Collaborator Author

tdstein commented Dec 13, 2024

@schloerke - I want to expose Package, Packages, ContentPackage, and ContentPackages so that they may be imported via from posit.connect import Package, Packages, ContentPackage, ContentPackages. Do you know if it's better practice to make them public here or to keep them private and then rename them as public in src/posit/connect/__init__.py?

@tdstein tdstein requested review from schloerke and toph-allen and removed request for schloerke December 13, 2024 19:04
@tdstein tdstein marked this pull request as ready for review December 13, 2024 19:04
@schloerke
Copy link
Collaborator

schloerke commented Dec 13, 2024

I'm up for removing the leading _ from all the names. They are public or things package authors might use and so they should be available in the posit/connect/__init__.py file.

I would argue for renaming almost every file to start with a _ though. Ex: jobs.py to _jobs.py. Variables like JobTag are currently considered public and stable. (I would keep the ./external folder and files as they are as their use case is different than most of the code.)

With the renaming of the jobs file (for example), it would be good to expose Jobs and Job through the init file just like Package and Packages.


If you still like the cadence of from posit.connect.jobs import Job, Jobs (which I also enjoy), we could turn the file structure into something like

./jobs
    | - __init__.py # Exposes `Job` and `Jobs`
    | - _jobs.py # The current content of `./jobs.py`

This would hide all variables and only expose what you want to expose through posit.connect.jobs. We could even re-export those variables in the top level posit.connect where appropriate.


Repeating with Packages...

./packages
    | - __init__.py # Exposes `Packages` and `Package`
    | - _packages.py # The current content of `./packages.py`

Links to a py-shiny implementation of this:
https://github.com/posit-dev/py-shiny/blob/7ae7725e2dc082fb02b4bcfac44c60669dc8d501/shiny/session/__init__.py where Session is from https://github.com/posit-dev/py-shiny/blob/7ae7725e2dc082fb02b4bcfac44c60669dc8d501/shiny/session/_session.py and also re-exported at the top level at https://github.com/posit-dev/py-shiny/blob/7ae7725e2dc082fb02b4bcfac44c60669dc8d501/shiny/__init__.py

It allows for both from shiny import Session and from shiny.session import Session (and internally we can also call ./session/_session import Session)

@schloerke
Copy link
Collaborator

I could also see a benefit of a ./protocols.py or ./types.py file that re-exports all of the Protocol classes. This might be useful to help drive home that they are not implementation classes, but just types.

@tdstein tdstein merged commit b182233 into main Dec 16, 2024
35 checks passed
@tdstein tdstein deleted the tdstein/packages-add-fetch branch December 16, 2024 15:12
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.

2 participants