Skip to content

Add some basic typechecking support#162

Open
davidshepherd7 wants to merge 11 commits intopython-smpplib:masterfrom
davidshepherd7:basic-typechecking-support
Open

Add some basic typechecking support#162
davidshepherd7 wants to merge 11 commits intopython-smpplib:masterfrom
davidshepherd7:basic-typechecking-support

Conversation

@davidshepherd7
Copy link
Contributor

Hi!

Are you interested in having type checking for this project? I've been using some type definitions for my own work using the library and figured it would be nice to merge them upstream. So I started out doing that but got a bit carried away and added types for almost everything except the Command objects.

I've mostly tried to avoid modifying any code in order to add typechecking. The major exception is that I removed python 2 support because it makes specifying types much easier. I hope that's ok!

Other interesting changes:

  • I added a few assertions on the type of things where we would crash anyway, these keep the type checker happy.
  • I was a bit nervous about the (minor) PDU parsing/generating code changes, so I wrote some more unit tests for it.
  • I needed to re-implement Fixing pdu.generate() encoding #132 to get the tests working.

If there are specific changes that you don't want I'm happy to send separate smaller PRs. I did it all as one here because some of the changes rely on each other.

If you're happy with this then I can also try to work out a way to add types for the command objects, I think it will be possible with a bit of manual work (and/or keyboard macros).

python 3.4 is causing the tests to fail, and it's EOL so I guess it's ok to
remove it?

python 2 - I'd like to try out adding type checking support and I think that's a
lot harder with python 2, so if it's ok I'd like to remove support.
The only change to runtime code introduced here should be skipping the
initialisation of Client.port to None. I'm pretty confident that this is fine
since __init__ always initialises it to an int.
This would crash anyway because the `value` variable doesn't exist, but with the
explict assertion it typechecks (and is easier to debug).
…me output

Writing tests and adding types flagged up this issue where sometimes the
generate functions would use "\0" instead of b"\0".
@eigenein
Copy link
Collaborator

eigenein commented Sep 3, 2021

@podshumok What's the status of Python 2 support again?

@code-of-kpp
Copy link
Member

Wow, super cool work! I'd really like to have it merged.

We had an idea of supporting Python2 because we assumed (just based on the types of the questions and code samples people sent us) that the library is used in many very outdated environments and since we were not doing anything actively, we decided to support Py2 until PyPy2 (last of its kind python2 implementation) is in active development.

I think, we still can have it, if we'd move types to python-stubs. This has some benefits on it's own (they don't affect runtime at al) and is more in line with current practice (providing types-* package to install dependencies types, for example).

mypy's stubgen will do the job I think (run it on your current version, it will create the files, revert the original files) and you only need to re-apply the runtime-affecting changes).

If even with stub files we'll wouldn't be able to support Py2, I vote for dropping it's support.

@eigenein
Copy link
Collaborator

eigenein commented Sep 8, 2021

@podshumok Can we check in the pypi.org how much downloads we get from Python 2? I'd frankly vote for just dropping the support for it.

@code-of-kpp
Copy link
Member

I don't have big query set up and we mostly distribute source package, so data might not even be there...

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.

3 participants