Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: rework ops.main type hints to allow different flavours (callable class) #1345

Merged
merged 33 commits into from
Sep 3, 2024

Conversation

dimaqq
Copy link
Contributor

@dimaqq dimaqq commented Aug 28, 2024

Merge readiness check

  • implementation
  • tests
  • docs
  • re-export some symbols

Release readiness check

Slated for ops==2.17.0

https://warthogs.atlassian.net/browse/CHARMTECH-232

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.

Looks good to me - a few minor queries/suggestions.

I did not review any of the code in _main.py other than main(), assuming that it was unchanged from main.py - if that's not the case and there are tweaks there too, could you please note that in the PR description and I'd want to re-review.

Speaking of the PR description: I assume it's because there are three competing solutions in different PRs, but it's currently empty: could you please fix that?

test/test_main_invocation.py Outdated Show resolved Hide resolved
test/test_main_type_hint.py Outdated Show resolved Hide resolved
ops/main.py Show resolved Hide resolved
ops/main.py Show resolved Hide resolved
ops/main.py Show resolved Hide resolved
ops/_main.py Outdated Show resolved Hide resolved
ops/_main.py Outdated Show resolved Hide resolved
ops/_main.py Outdated Show resolved Hide resolved
@dimaqq
Copy link
Contributor Author

dimaqq commented Aug 29, 2024

py:func reference target not found: ops.main [ref.func] I'm at a loss how to link to new top-level ops.main()

ops/__init__.py Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
@tonyandrewmeyer
Copy link
Contributor

py:func reference target not found: ops.main [ref.func] I'm at a loss how to link to new top-level ops.main()

Can you just put in a custom inline target and link to that instead? You'd lose the auto formatting, but at least it would be a link. Like:

def main(...):
    """_`ops.main()` is the thing you want to call."""

Charms should call ops.main()_ in order to handle the event.

@dimaqq
Copy link
Contributor Author

dimaqq commented Aug 29, 2024

Screenshot 2024-08-29 at 15 09 09

Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Couple of nit comments, comment about re-exporting those few names in main.py, and question whether we need test_main_type_hint.py.

ops/__init__.py Outdated Show resolved Hide resolved
ops/main.py Show resolved Hide resolved
ops/main.py Show resolved Hide resolved
test/test_main_invocation.py Show resolved Hide resolved
test/test_main_type_hint.py Outdated Show resolved Hide resolved
@benhoyt
Copy link
Collaborator

benhoyt commented Aug 30, 2024

Docs look good though -- thanks!

@dimaqq dimaqq requested a review from benhoyt September 2, 2024 05:12
@dimaqq
Copy link
Contributor Author

dimaqq commented Sep 2, 2024

I've decided to re-export the set of symbols that Scenario 6 uses specifically.

Rationale: many charms use Scenario and we shouldn't potentially break a whole bunch of charms if they pin dependencies asymmetrically (compatible ops, exact scenario); and to decouple Scenario 6 update from this PR in calendar time.

@james-garner-canonical
Copy link
Contributor

I've given things a read through and it looks good to me, with the caveat that I'm still not familiar enough with the code base for that to signify a lot. I raised a couple of questions on test_main_type_hint.py just about the type checking tests.

@dimaqq dimaqq merged commit cdfd10f into canonical:main Sep 3, 2024
32 checks passed
@dimaqq dimaqq deleted the fix-main-with-depr-class branch September 3, 2024 06:06
github-merge-queue bot pushed a commit to canonical/charmcraft that referenced this pull request Sep 21, 2024
Jira: https://warthogs.atlassian.net/browse/CHARMTECH-223
Parent: https://warthogs.atlassian.net/browse/CHARMTECH-219

The idea is to validate that the charm initialises the operator
framework correctly:
- charmcraft analyse would validate the presence of the `ops.main(...)`
call with this PR
   - any conceivable import style is supported
- ops library type hints are improved
canonical/operator#1345
   - charmers will no longer need to slap `# type: ignore` on the call
- we'll be relying on charmers' static type analysis to ensure correct
arguments to `ops.main`

---------

Co-authored-by: Alex Lowe <alex.lowe@canonical.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants