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

Improve .model type hint #143

Open
dimaqq opened this issue Sep 30, 2024 · 3 comments
Open

Improve .model type hint #143

dimaqq opened this issue Sep 30, 2024 · 3 comments

Comments

@dimaqq
Copy link
Contributor

dimaqq commented Sep 30, 2024

It seems that most charms' integration tests assume that ops_test.model is truthy (6592 call sites):

async def setup_env(ops_test: OpsTest):
    await ops_test.model.set_config(...)

And a few ensure that it's truthy (46 call sites):

async def test_integrate(ops_test: OpsTest):
    assert ops_test.model is not None
    await asyncio.gather(
        ops_test.model.integrate("mimir:s3", "s3"),
        ...
    )

I didn't find a single test that's conditional on the model being or not being there. We're tracking ~160 charming repos in https://github.com/tonyandrewmeyer/charm-analysis

Currently, it's typed so:

    @property
    def model(self) -> Optional[Model]:
        """Represents the current model."""
        current_state = self.current_alias and self._models.get(self.current_alias)
        return current_state.model if current_state else None

I'm proposing to type it as Model and raise if the model is not available.

@addyess
Copy link
Member

addyess commented Oct 7, 2024

@dimaqq Do you think it constitutes a major rev on this package? It feels kinda sneaky to keep subtly changing the API. I get the reasoning to begin to add a ModelMissing error -- but it feels a bit icky to change the API.

@dimaqq
Copy link
Contributor Author

dimaqq commented Oct 7, 2024

I’m about 98% confident that it’s a minor or micro version bump.

I haven’t found a charm that relies .model being nullable.

If there is such a charm, that’s a different story.

@addyess
Copy link
Member

addyess commented Oct 7, 2024

@dimaqq if it's critical you can post a PR and @ca-scribner or myself would review.

if you can wait a bit -- maybe i can get to it in the next few weeks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants