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 #16

Closed

Conversation

thomas-phillips-nz
Copy link
Contributor

Since we're adding :type: to parameters, we might as well use Python's native type hints 🤷

@kxtells
Copy link
Owner

kxtells commented Sep 28, 2023

First of all, thanks for the PR and your involvement :-D

My first instinct on this is that is not valid python2.X 🤣 (yeah, ancient, but the two places I worked on that used this tenma-serial still use the 2.7 toolchain).
On the other hand, this PR made me realize that I let slip a yield from in a previous PR, so the current version is not valid 2.7 either 🤷‍♂️ , so that is not a problem.

My second instinct is that I'm not a fan of type hinting in python (we could open a big discussion here probably hehe). In this case, I just added :type: in the docstring yesterday so when the documentation gets released it has this extra information on what is expected.

Other than that, I see no reason not to go ahead with type hinting apart from me not being to fond about them, which is a big reason :-D

@thomas-phillips-nz
Copy link
Contributor Author

thomas-phillips-nz commented Sep 28, 2023

Yeah, I forgot about 2.7 compatibility. That was added in #10 by me hehe.
You could always add a tag at the commit before #10 was merged for 2.7

Actually, we might be able to change that yield from into something else to make it 2.7 compatible. I'll have to try it out

It's your prerogative to reject the PR if you don't like your hints. I just figured it's more "standard" rather than including types in the do strings.

Either way, the time parameter for the auto step functions is a float, so I can create a smaller PR with just that fix in the docstring

@thomas-phillips-nz
Copy link
Contributor Author

Will leave this for now, I have made a new PR to restore python 2.7 support: #17
I can imagine it might be annoying for existing labs stuck on python 2.7 to be dropped by an update when we don't actually need to drop support.

@kxtells
Copy link
Owner

kxtells commented Sep 29, 2023

@thomas-phillips-nz. Yeah, the python2.7 thing is annoying but at the same time for this tiny code-base I think we can keep it alive as both 2.7 and 3.X valid code.

Thanks again for your efforts :-D

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