Skip to content

Conversation

@wch
Copy link
Collaborator

@wch wch commented Apr 12, 2024

This PR makes some typing fixes, and also adds some runtime checks that ce.remote_server is a RSConnectServer.

Comment on lines 2213 to 2214
if not isinstance(ce.remote_server, RSConnectServer):
raise RSConnectException("This command requires a Posit Connect server.")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used a pretty generic exception message here, but am willing to customize it for each command, if you think that would help.

For example, in this case, it could be:

"`rsconnect content describe` requires a Posit Connect server."

@wch wch requested a review from mmarchetti April 12, 2024 04:59
@github-actions
Copy link

github-actions bot commented Apr 12, 2024

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
4720 3520 75% 0% 🟢

New Files

No new covered files...

Modified Files

File Coverage Status
rsconnect/actions_content.py 65% 🟢
rsconnect/main.py 61% 🟢
TOTAL 63% 🟢

updated for commit: 2cf5412 by action🐍

@wraps(func)
def wrapper(*args: P.args, **kwargs: P.kwargs):
def failed(err: str):
def failed(err: str) -> Never:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The explicit Never return helps pyright recognize that the calls to failed() will never return.

Copy link
Contributor

@mmarchetti mmarchetti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with the more generic messages, but the specific ones would be a nice touch.

@wch wch merged commit 855c762 into master Apr 12, 2024
@wch wch deleted the typing-fixes branch April 12, 2024 15:28
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.

3 participants