Improve type checking: return ‘Reader’ from open_database() #88
Conversation
Mypy reports attr-defined errors (via no_implicit_reexport) because the ‘maxminddb’ module uses implicit re-exports. Adding __all__ makes the exported API surface explicit, which avoids the type checking errors.
|
this is a follow-up to #87 to further improve the type checking story for this project |
|
(gentle ping @oschwald since you looked into this before) |
|
yes i don't like it either, to be clear. see my comment added in the code. it seemed nontrivial to introduce a base class for use both in python and in the c extension, and have stubs for that, so i took the easy way out. baby steps, etc |
|
oh, an alternative approach would be to fully hide the c extension's Reader inside the Python Reader, making its existence fully an implementation detail, from a public API perspective. but that's a change i didn't want to make undiscussed (and i expect it would cascade into test suite refactorings) |
|
If I am understanding the proposal, I suspect hiding the extension in that way would also have runtime performance costs. It looks like |
|
yeah, it would be an indirection. (unless you count very ugly dynamically reassignment of methods as an option). hence definitely not something to blindly turn into a pr without thinking and discussion. typing from phone, will look into pylint later |
This exposes the ‘Reader’ class under its own name (instead of an alias) and uses it as the return type for open_database(). In case the C extension is used, cast that to the same ‘Reader’ type for type checking purposes, since they share an identical API. The benefits are that this avoids an unnecessary Union return type (see e.g. python/mypy#1693) and doesn't require internal types to be exposed with confusing names for type checking purposes only. The ‘Reader’ compat helper is unnecessary and can be dropped: the Python Reader is always available and API compatible. While at it, simplify and isort the imports.
2665784 to
e8a521b
Compare
|
local pylint reported different things, and apparently github actions don't auto-trigger for me. i've added some changes but can't see the result. |
|
Related to the I think this happened due to the removal of |
|
relying on the implicit side effect of an unused import is not ideal 🙃 see the description of the commit i just pushed: 544379f |
|
import maxminddb.extension as _extension |
|
typing from phone, but i think "from . import submodule" is fine? does it actually not work? 🤔 (your suggestion is invalid python i think?) |
|
pylint has various bugs in this area it seems |
|
another one |
|
I believe the suggestion is valid Python. It is the standard way to import a module under another name. Your version does work. I don't have a strong opinion if you prefer to disable the linter here, although I have to admit that I am not sure I have seen a module imported that way elsewhere. |
|
it's really rather common to |
|
As I mentioned, I am fine with disabling the pylint check for that line. I don't have a strong preference. |
If the extension module cannot be imported, the fallback path would trigger ‘NameError: name 'maxminddb' is not defined’. While at it: - Use an underscored name for the optional extension import to hide it a bit from view (e.g. autocomplete in interactive Python shells) - Use relative imports not unnecessarily repeat the module name everywhere
544379f to
7b28aa6
Compare
|
i tried to reproduce the pylint error locally, and at first i could not. it turns out pylint only complains if it is executed against code that does not have the extension. the github action does not install the module before running pylint. if the extension module actually exists, the error does not appear. locally i have a anyway, the simplest is to just add the ignore so that it works in all scenarios. i've amended the change to do this. |
This exposes the ‘Reader’ class under its own name (instead of an alias)
and uses it as the return type for open_database(). In case the C
extension is used, cast that to the same ‘Reader’ type for type checking
purposes, since they share an identical API.
The benefits are that this avoids an unnecessary Union return type
(see e.g. python/mypy#1693) and doesn't
require internal types to be exposed with confusing names for type
checking purposes only.
The ‘Reader’ compat helper is unnecessary and can be dropped: the Python
Reader is always available and API compatible.
While at it, simplify and isort the imports.