Closed jicruz96 closed 1 month ago
Thanks for your contribution! I'll review and test the changes in the beginning of next week. No worries about the workflow fail. I may replace the current workflow anyway before a new release.
I reviewed the code and it looks good to me. Thanks for your effort so far! The tests are running, but a mypy check returned some errors. Would you look into these?
$ mypy --install-types --non-interactive geonamescache tests
geonamescache/__init__.py:40: error: Incompatible types in assignment (expression has type "dict[str, Any]", variable has type "dict[Literal['AF', 'AN', 'AS', 'EU', 'NA', 'OC', 'SA'], Continent] | None") [assignment]
geonamescache/__init__.py:42: error: Incompatible return value type (got "dict[Literal['AF', 'AN', 'AS', 'EU', 'NA', 'OC', 'SA'], Continent] | None", expected "dict[Literal['AF', 'AN', 'AS', 'EU', 'NA', 'OC', 'SA'], Continent]") [return-value]
geonamescache/__init__.py:56: error: Incompatible return value type (got "dict[str, USState]", expected "dict[Literal['Alaska', 'Alabama', 'Arkansas', 'Arizona', 'California', 'Colorado', 'Connecticut', 'District of Columbia', 'Delaware', 'Florida', 'Georgia', 'Hawaii', 'Iowa', 'Idaho', 'Illinois', 'Indiana', 'Kansas', 'Kentucky', 'Louisiana', 'Massachusetts', 'Maryland', 'Maine', 'Michigan', 'Minnesota', 'Missouri', 'Mississippi', 'Montana', 'North Carolina', 'North Dakota', 'Nebraska', 'New Hampshire', 'New Jersey', 'New Mexico', 'Nevada', 'New York', 'Ohio', 'Oklahoma', 'Oregon', 'Pennsylvania', 'Rhode Island', 'South Carolina', 'South Dakota', 'Tennessee', 'Texas', 'Utah', 'Virginia', 'Vermont', 'Washington', 'Wisconsin', 'West Virginia', 'Wyoming'], USState]") [return-value]
geonamescache/__init__.py:56: error: Argument 1 to "get_dataset_by_key" of "GeonamesCache" has incompatible type "dict[Literal['AK', 'AL', 'AR', 'AZ', 'CA', 'CO', 'CT', 'DC', 'DE', 'FL', 'GA', 'HI', 'IA', 'ID', 'IL', 'IN', 'KS', 'KY', 'LA', 'MA', 'MD', 'ME', 'MI', 'MN', 'MO', 'MS', 'MT', 'NC', 'ND', 'NE', 'NH', 'NJ', 'NM', 'NV', 'NY', 'OH', 'OK', 'OR', 'PA', 'RI', 'SC', 'SD', 'TN', 'TX', 'UT', 'VA', 'VT', 'WA', 'WI', 'WV', 'WY'], USState]"; expected "dict[str, USState]" [arg-type]
geonamescache/mappers.py:27: error: Missing return statement [return]
geonamescache/mappers.py:34: error: TypedDict key must be a string literal; expected one of ("areakm2", "capital", "continentcode", "currencycode", "currencyname", ...) [literal-required]
Found 6 errors in 2 files (checked 9 source files)
I reviewed the code and it looks good to me. Thanks for your effort so far! The tests are running, but a mypy check returned some errors. Would you look into these?
@yaph I've handled the remaining mypy errors by loosening the type hints a bit for the helpers in 01e8341 and by defining specific function overloads for geonamescache.mappers.country
in commits d0cc123 and 89ae766 (edit, also commit 89ae766)
mypy --install-types --non-interactive geonamescache
Success: no issues found in 5 source files
The new function overloads also make the usage of country()
much better at type-inference and type checks too:
Thanks for your contribution! I'll review and test the changes in the beginning of next week. No worries about the workflow fail. I may replace the current workflow anyway before a new release.
Sounds good! fwiw I would expect that adding a pip install -e .
to your github workflow file should fix the currently broken tests if you choose not to replace the existing workflow
Thanks so much for your work @jicruz96! While there are no breaking changes for the end user, I'll create a new major release soon.
closes #41
Changes
geonamescache.types
usingtyping.TypedDict
typing-extensions
added as package dependency to ensure type hints work for all Python versions >=3.6*This will offer a more user-friendly experience in IDEs, including auto-complete and type checking (if the user has configured these).
Notes
*If you do not want any dependencies added to the package, I can provide an alternate PR
That alternate PR would use
sys.version_info
to check the python version before assigning types, and then use only thetyping
features available for each respective python version. I would ensure to support up to 3.8, and can support up to 3.6 if requested.On formatting:
There was no formatting or linting config beyond the .editorconfig, so I took my best guess with line breaks and line lengths.