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

add type hints #42

Merged
merged 7 commits into from
Jul 25, 2024
Merged

add type hints #42

merged 7 commits into from
Jul 25, 2024

Conversation

jicruz96
Copy link
Contributor

@jicruz96 jicruz96 commented Jul 18, 2024

closes #41

Changes

  • Type hints added to all declarations.
  • loaded data is typed in geonamescache.types using typing.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 the typing 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.

@@ -7,54 +7,62 @@

import json
import os
from typing import Any, Dict, List, Mapping, Optional, Tuple, TypeVar
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ NOTE: I did not update the version number (see line 3 of this file) and I leave the new version number up to your discretion.

I would recommend this be treated as a patch, as the underlying functionality hasn't changed, although a minor update would make sense too given the addition of a dependency.

@yaph
Copy link
Owner

yaph commented Jul 19, 2024

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.

@yaph
Copy link
Owner

yaph commented Jul 23, 2024

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)

@jicruz96
Copy link
Contributor Author

jicruz96 commented Jul 25, 2024

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:
Recording2024-07-25at14 44 36-ezgif com-resize

@jicruz96
Copy link
Contributor Author

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

@yaph yaph merged commit e17902b into yaph:master Jul 25, 2024
@yaph
Copy link
Owner

yaph commented Jul 25, 2024

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.

@yaph yaph self-assigned this Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE REQUEST] type hints?
2 participants