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

Feature/improve typing slightly #5

Merged

Conversation

coldino
Copy link
Contributor

@coldino coldino commented Aug 3, 2023

Clean up some minor typing issues.

pyproject.toml Outdated
@@ -8,7 +8,7 @@ readme = "README.md"
homepage = "https://github.com/harrymander/dataclasses-struct"
repository = "https://github.com/harrymander/dataclasses-struct"
documentation = "https://github.com/harrymander/dataclasses-struct/blob/main/README.md#usage"
packages = [{include = "dataclasses_struct"}]
packages = [{ include = "dataclasses_struct" }, { include = "dataclasses_struct/py.typed" }]
Copy link
Owner

Choose a reason for hiding this comment

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

This is not needed since Poetry automatically includes everything in the package source dir. (You can check by installing in a virtualenv and the py.typed file should be in the lib/site-packages/dataclasses_struct dir)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. Was basing this on a few examples that specifically add it in setup.py as it isn't a normal source file, but I guess Poetry knows about it and included it already.

@harrymander
Copy link
Owner

harrymander commented Aug 3, 2023

I don't use pylance/VS Code regularly for Python dev, so I assume the provided mypy extension is not compatible? I originally had tried annotating _make_class to return a protocol but mypy wasn't able to pick it up for decorated classes, which seems to be related to this longstanding issue python/mypy#3135

If pylance supports it out the gate then that's cool. Do you know if it possible to add tests for this similar to how I have done for mypy (doesn't need to be in this PR)?

BTW - thanks for all the PRs, will get round to the others soon when I get the chance :)

@@ -275,7 +280,7 @@ def from_packed(cls, data: bytes) -> cls_type:

def _make_class(
cls: type, endian: str, allow_native: bool, validate: bool
) -> type:
) -> type[DataclassStructProtocol]:
Copy link
Owner

Choose a reason for hiding this comment

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

This needs to be Type to support Python 3.8 - see CI error.

@coldino
Copy link
Contributor Author

coldino commented Aug 3, 2023

I don't use pylance/VS Code regularly for Python dev, so I assume the provided mypy extension is not compatible? I originally had tried annotating _make_class to return a protocol but mypy wasn't able to pick it up for decorated classes, which seems to be related to this longstanding issue python/mypy#3135

The mypy extension is not useful for PyLance. I'm trying to improve things as PyLance is both edit-time type checking (rather than just on-save) and provides all the autocomplete information, so having it understand these classes would be ideal. Currently using is_dataclass_struct is the best workaround as the TypeGuard works well.

It appears the @dataclass_transform is discarding the relevant information. Unfortunately when I experimentally added a second decorator to add the protocol type information it still wasn't a complete solution - it meant you could no longer inherit from one of these structs.

BTW - thanks for all the PRs, will get round to the others soon when I get the chance :)

Thanks for the project! Took a while to find and add the changes I need, but certainly a lot better than the alternative of making my own.

* Avoids a `struct` name clash that was confusing PyLance
* Add `from_packed` and `pack` to `DataclassStructProtocol`
* Expose `DataclassStructProtocol` in top-level module
@harrymander harrymander force-pushed the feature/improve-typing-slightly branch from 5f0328e to 9a9b4b7 Compare August 3, 2023 10:07
@harrymander harrymander merged commit 9a9b4b7 into harrymander:main Aug 3, 2023
@coldino coldino deleted the feature/improve-typing-slightly branch June 14, 2024 11:20
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.

2 participants