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 mypy type linting #177

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Add mypy type linting #177

wants to merge 14 commits into from

Conversation

Drvanon
Copy link
Contributor

@Drvanon Drvanon commented Mar 18, 2023

This should add the desired unit testing for typing.

The pull request is based on weditors pull request for type hints.

TODO:

  • Check reason for no_wrap failing.
  • Make a decision for overloading errors from mypy.
  • Edit github files to include a mypy step.
  • Fire up discussion on using the typing.Sequence type for use in Sequence._base_sequence
    • It would fix the iterable but should have length dilemma.
  • Add typing for no_wrap.
  • Get rid of implicit optionals.

@Drvanon
Copy link
Contributor Author

Drvanon commented Mar 18, 2023

That last commit was generated by GitHub after i fat fingered on GitHub mobile. I will have to figure out what exactly it did.

@Drvanon
Copy link
Contributor Author

Drvanon commented Mar 25, 2023

So I noticed that I could fix the poetry errors by simply bumping the pylint dependency. This made made the sub-dependency on typed-ast go from 1.4.2 (which causes the installation issues) to 1.5.4.

Edit: Apparently this causes a merge conflict, that I don't really know what to do with right now. Open to input here.

@Drvanon
Copy link
Contributor Author

Drvanon commented Mar 25, 2023

The nose testing library is no longer maintained and is incompatible with python 3.10. I am going to solve this for now by including a maximum python version of 3.9, but this is clearly not a sustainable solution.

@Drvanon
Copy link
Contributor Author

Drvanon commented Mar 25, 2023

I am thinking that it might make more sense to do the "maintenance" commits in a separate PR. Especially because of the depreciation of nosetest. I personally favor pytest, but I read that there also is a nose2. What is your opinion about this?

Edit: apparently this was dealth with in commits since weditor's PR. I am now looking to do a maintenance update from the current master and then merge weditors PR into that.

@Drvanon
Copy link
Contributor Author

Drvanon commented Mar 26, 2023

I reworked master to be based of the maintenance PR (#178) so that it would be easier to run the tests, linting etc.

@stale
Copy link

stale bot commented May 18, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label May 18, 2023
@stale stale bot removed the stale label May 19, 2023
@MartinBernstorff
Copy link

@EntilZha I'm looking into using pyfunctional, and getting type checking for the most common operations (i.e. application of anonymous functions with .map, filter and reduce) would be lovely. Seems like this PR does that based on my local testing, although it doesn't fix all type problems. This means a mypy linting step would keep failing.

We could add a typeguard test or similar, that does runtime type checking of a subset of the available methods? I think that'd be a big improvement over the current state of type hinting, without requiring too much work, and could get started on that work ASAP.

Is that an acceptable MVP for getting this PR?

@MartinBernstorff
Copy link

An alternative, if you'd prefer to avoid runtime type checking in the library code, would be asserting something like isinstance(Sequence[int])?

@MartinBernstorff
Copy link

@EntilZha, just pinging this once! Alternatively, I might make a simple implementation of PyFunctional as a fork :-)

@EntilZha
Copy link
Owner

Hey, sorry for the delay @MartinBernstorff, was camping/busy.

To help maximize chance we get some more typing into pyfunctional:

  1. Even if it is failing, I'd like to have a PR that adds mypy as an automatic checker. Ideally, PRs after that should reduce the number of mypy errors and the mypy output should show that the introduced type hints in any given PR are correct. The main thing I'd like to avoid is not having any automated way to track improvement in type coverage.
  2. PRs should be as minimal as possible and do one thing. E.G., I would break this PR into: (a) a bump in package dependencies that maintain passing tests (i.e., upgrade black and run the new black), (b) changes related to adding new type hints, (c) changes unrelated to package upgrades or adding type hints.
  3. I don't think runtime checks are the way to go, since it incurs a performance penalty. If there were a way to do optional runtime checks that don't incur significant perf penalty, could be an option, maybe.
  4. As is, the PR has merge conflicts so can't actually merge. I'd guess that most of these aren't too difficult to resolve and mainly involve a rebase to main.

My suggestion on bang for buck on typing is to take the generic type changes and make a clean PR with them on main (i.e., the _T_co = TypeVar lines). My guess is that this would go a long way towards helping automatic type inference.

@EntilZha
Copy link
Owner

FYI, I made a quick PR that adds mypy with it passing, so should be able to make a PR with just adding types now.

@MartinBernstorff
Copy link

Awesome! I'll take a look in the coming days 👍

@Shoggomo
Copy link

Shoggomo commented Jan 4, 2024

How is this coming along? Would be awesome to have proper typing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants