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 some type annotations, drop support for Python 3.6 #9

Closed
wants to merge 3 commits into from

Conversation

mike8699
Copy link
Contributor

@mike8699 mike8699 commented Sep 7, 2023

This PR adds a few basic type annotations to the bmg module. The main motivation behind this is to improve developer experience when using ndspy in an IDE. For example, consider the following snippet of code:

from ndspy import bmg

bmg_file = bmg.BMG(<some data>)

for message in bmg_file.messages:
  print(message.stringParts)

The BMG.messages instance variable is untyped in ndspy, so an IDE wouldn't be able to offer a code suggestion for .stringParts, and the user would have to consult the docs for the exact attributes of message.

@RoadrunnerWMC are you open to this change/more changes like it? I've been adding some type annotations like these to my local copy of ndspy to aid my own development, but if they can be of use more generally then I'm happy to upstream them.

One additional note - I also dropped support for Python 3.6 in this PR to enable usage of the from __future__ import annotations import, which allows usage of more modern typing syntax like list[T], tuple[T, T], X | Y etc. for Python 3.7+ over typing.List[T], typing.Tuple[T, T], typing.Union[X, Y], etc. I figured this might be acceptable since 3.6 (and 3.7 in fact) are EOL. But if you'd prefer not to do that, and do want to accept this PR, let me know and I can change it to the 3.6-compatible types.

@RoadrunnerWMC
Copy link
Owner

Thanks for the PR!

I actually add type annotations to all new Python code I write nowadays, though it's mainly for readability/documentation since I don't personally use an IDE with features like you're describing. So I'm definitely in favor of this.

I think dropping support for EOL Python versions is fine. (No need to drop support for 3.7 until we actually need something from it, though.) If anyone does need them for some reason, please open an issue so we can talk about it.

The only thing I don't like about this PR is that it doesn't annotate everything in the module. Are you planning to add the rest in a follow-up PR, or should I?

@mike8699
Copy link
Contributor Author

The only thing I don't like about this PR is that it doesn't annotate everything in the module. Are you planning to add the rest in a follow-up PR, or should I?

Great! Yeah I can do the rest of the module, I was just testing the waters with this PR before I did the whole thing. I'll make a follow on PR based on this one that adds type annotations for the rest of the bmg module.

@RoadrunnerWMC
Copy link
Owner

Closing in favor of #10, which includes these changes + more.

@mike8699 mike8699 deleted the patch-1 branch September 21, 2023 00:46
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