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

Investigate whether Optional members of ops/pebble.py TypedDicts can be NotRequired instead #1405

Open
james-garner-canonical opened this issue Oct 2, 2024 · 6 comments
Labels
investigate Needs further investigation - do we want to do this? low priority Non-urgent issue we may (or may not) consider later small item type hints

Comments

@james-garner-canonical
Copy link
Contributor

james-garner-canonical commented Oct 2, 2024

Several TypedDicts in ops/pebble.py represent values returned from Pebble. Generally pebble will simply omit an optional field if it is not set (but we need to confirm this for each one). Due to limitations of typing on older Python versions, these TypedDicts sometimes use total=False to coarsely describe this, as well as annotating some values as Optional (i.e. can be None).

There are two components here.

  1. Can we use NotRequired from typing_extensions cleanly while supporting older python versions, e.g. putting the NotRequired import behind if TYPE_CHECKING and using stringified type annotations? This would allow us to explicitly mark which keys are required and which are not instead of using total=False.
  2. For public TypedDicts, is it a supported pattern for users to pass None as a synonym for the entry being missing? When a dict only comes from Pebble, we can use NotRequired[some_type], but when a dict can be constructed by a user, we may need/want to use NotRequired[Optional[some_type]] instead to let them use None for values they don't want to set.

Related: investigate the use of Optional values in Check. CheckDict has several Optional members which will simply be missing if they're not set when coming from Pebble. The Check constructor annotates these values as Optional, but sets some of them to empty strings instead if they're missing. Check.to_dict then filters out any Falsey values, including empty strings and None. This may be a good opportunity to tidy this up a little bit.

@james-garner-canonical james-garner-canonical added small item type hints low priority Non-urgent issue we may (or may not) consider later investigate Needs further investigation - do we want to do this? python labels Oct 2, 2024
@benhoyt
Copy link
Collaborator

benhoyt commented Oct 2, 2024

Thanks for opening this. Getting types right is hard! :-)

FWIW, I think a python label is of limited value, as it's going to be on pretty much every (non-docs) issue here, so feels like just label noise. I think type hints is fine.

@james-garner-canonical
Copy link
Contributor Author

Worth removing the python label from the issue tracker? I see it has only been used by dependabot a few times some months ago.

@tonyandrewmeyer
Copy link
Contributor

Worth removing the python label from the issue tracker? I see it has only been used by dependabot a few times some months ago.

Dependabot will (re)create the label because there's more than one language/subsystem being updated (Python and GitHub actions). We can disable that in the dependabot config if we don't want the default behaviour.

@benhoyt
Copy link
Collaborator

benhoyt commented Oct 2, 2024

Oh, thanks -- I didn't realise that was a dependabot-created label. If we can remove and get dependabot to not recreate it easily, that's fine, but not a big deal nor worth spending much time on.

@james-garner-canonical
Copy link
Contributor Author

I've gone with the easy solution of updating the python label description to 'dependabot PR that updates python dependencies'.

@tonyandrewmeyer
Copy link
Contributor

Oh, thanks -- I didn't realise that was a dependabot-created label. If we can remove and get dependabot to not recreate it easily, that's fine, but not a big deal nor worth spending much time on.

It should be straightforward: #1407

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
investigate Needs further investigation - do we want to do this? low priority Non-urgent issue we may (or may not) consider later small item type hints
Projects
None yet
Development

No branches or pull requests

3 participants