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: rework ops.main type hints to allow different flavours (protocol) #1340

Closed
wants to merge 19 commits into from

Conversation

dimaqq
Copy link
Contributor

@dimaqq dimaqq commented Aug 26, 2024

  • our project tests
  • super-tox
  • deprecation docs

@dimaqq dimaqq force-pushed the fix-main-with-deprecation branch from 22374b1 to 0e54cf6 Compare August 28, 2024 21:36
@dimaqq dimaqq changed the title chore: fix ops.main type and deprecate ops.main.main() fix: rework ops.main type hints to allow different flavours (protocol) Aug 28, 2024
@dimaqq dimaqq force-pushed the fix-main-with-deprecation branch from bdcffde to 6cb0d60 Compare August 28, 2024 22:11
@dimaqq
Copy link
Contributor Author

dimaqq commented Aug 28, 2024

Doc build fails with /code/operator/ops/main.py:docstring of ops.main:1: WARNING: duplicate object description of ops.main, other instance in index, use :no-index: for one of them

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?

@dimaqq dimaqq marked this pull request as ready for review August 28, 2024 22:25
Copy link
Contributor

@tonyandrewmeyer tonyandrewmeyer left a 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
Copy link
Contributor

Choose a reason for hiding this comment

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

More words expected?

Comment on lines +42 to +44
class IdleCharm(ops.CharmBase):
def __init__(self, framework: ops.Framework):
super().__init__(framework)
Copy link
Contributor

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'
Copy link
Contributor

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?

Comment on lines -35 to -37
from ops.charm import CharmMeta
from ops.jujucontext import _JujuContext
from ops.log import setup_root_logging
Copy link
Contributor

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Outdated?

@dimaqq
Copy link
Contributor Author

dimaqq commented Aug 29, 2024

Closing in favour of #1345

@dimaqq dimaqq closed this Aug 29, 2024
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