-
Notifications
You must be signed in to change notification settings - Fork 122
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: rework ops.main type hints to allow different flavours (protocol) #1340
Conversation
dimaqq
commented
Aug 26, 2024
•
edited
Loading
edited
- our project tests
- super-tox
- deprecation docs
22374b1
to
0e54cf6
Compare
bdcffde
to
6cb0d60
Compare
Doc build fails with I think it's because there's ops/main and ops/_main both with a docstring. Should we hide the legacy bit doc string? Or is there a way to rename it for docs only? |
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.
More-or-less the same comments as on the callable class version.
I'd probably want to re-review if this ends up being chosen.
|
||
def type_test_dummy(_arg: Callable[[Type[ops.CharmBase], bool], None]): | ||
""" | ||
Helper to verify that |
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.
More words expected?
class IdleCharm(ops.CharmBase): | ||
def __init__(self, framework: ops.Framework): | ||
super().__init__(framework) |
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.
Why not just use CharmBase
?
from ops.jujucontext import _JujuContext | ||
from ops.log import setup_root_logging | ||
|
||
CHARM_STATE_FILE = '.unit-state.db' |
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.
Probably not importable any more?
from ops.charm import CharmMeta | ||
from ops.jujucontext import _JujuContext | ||
from ops.log import setup_root_logging |
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.
Probably should still be able to import CharmMeta and setup_root_logging.
|
||
"""Main entry point to the framework. | ||
|
||
Note that this module is callable, and calls the :func:`ops.main.main` function. |
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.
Outdated?
Closing in favour of #1345 |