Repository navigation
Conversation
8bd6642 (yaph#42 - "add type hints") added type hints all of the methods of `GeonamesCache` except for `get_us_counties()`. There's no discussion of the method in the pull request and no indication in the code that the absense of type annotations is important. It is likely that it was omitted, because it was the one usage of `_load_data()` that was incompatible with the type that _it_ specified (`list` vs. `dict`). The specification of `dict` by that method is not necessary. Specifying an unbound `TypeVar` will accomodate the usage by `get_us_counties()`. Being an annotation, the change will have no runtime affect and only a minimal effect for those consumers using a static type checker: the unusual consumer that attempts to `del` a key or assigns a value with an incompatible type will receive errors from their type checker. Additions to the dictionary will be permitted, since `USCountry` is not `closed`. As adding the annotation will remove the need for a `cast` by consumers, it's a worthwhile amendment to the interface.
cweider
commented
Sep 19, 2026
| ) | ||
|
|
||
| TDict = TypeVar('TDict', bound=Mapping[str, Any]) | ||
| TData = TypeVar('TData') |
Author
There was a problem hiding this comment.
It might be good to underscore these (and get_dataset_by_key() as well), but that has an API effect and can get done at some later date.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
8bd6642 (#42 - "add type hints") added type hints to all of the methods of
GeonamesCacheexcept forget_us_counties(). There's no discussion of the method in the pull request and no indication in the code that the absense of type annotation is important.It is likely that it was omitted, because it was the one usage of
_load_data()that was incompatible with the type that it specified (listvs.dict). The specification ofdictby that method is not necessary. Specifying an unboundTypeVarwill accomodate the usage byget_us_counties().Being an annotation, the change will have no runtime affect and only a minimal effect for those consumers using a static type checker: the unusual consumer that attempts to
dela key or assigns a value with an incompatible type will receive errors from their type checker. Additions to the dictionary will be permitted, sinceUSCountryis notclosed.As adding the annotation will remove the need for a
castby consumers, it's a worthwhile amendment to the interface.