-
Notifications
You must be signed in to change notification settings - Fork 125
chore: change the get_filesystem context argument from Context to Context[Any] #2248
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
base: main
Are you sure you want to change the base?
chore: change the get_filesystem context argument from Context to Context[Any] #2248
Conversation
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.
Pull request overview
This PR updates the type annotations for the get_filesystem methods in two classes (Container and Storage) to use Context[Any] instead of the bare Context type. Since these methods only use generic Context methods that don't depend on the specific charm type parameter, using Any as the type parameter is appropriate and avoids requiring type: ignore comments at call sites.
Key changes:
- Updated type annotation for
Container.get_filesystem()parameter fromContexttoContext[Any] - Updated type annotation for
Storage.get_filesystem()parameter fromContexttoContext[Any] - Removed the entire
testing/tests/test_plugin.pytest file (unrelated to type annotations)
Reviewed changes
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| testing/tests/test_plugin.py | Entire test file deleted (appears unrelated to the PR's stated purpose) |
| testing/src/scenario/state.py | Updated get_filesystem method type annotations from Context to Context[Any] in both Container and Storage classes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
james-garner-canonical
left a comment
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.
Seems like a good fix to me. In addition to the covariance issue, another thing that might improve the typing UX of Context would be adding a default to the generic as well. I'll add a note to the issue.
dimaqq
left a comment
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.
I'm not the expert here.
Approving on the assumption that this doesn't appear to be breaking the state transition tests in the third-party charms that we track.
Neither of the
get_filesystemmethods make any use of the generic type of Context, but we need to provide a type to avoid having calls need atype: ignore, soAnyseems the appropriate type.Maybe #2242 should be done instead? But I think that can be done later, replacing this if needed.