[WIP] Type hints #131
[WIP] Type hints #131
Conversation
|
@eliotwrobson A few thoughts and questions: New merge conflictIt looks like the recent merge (from maybe #121) introduced a merge conflict in the nfa.py module. Just wanted to let you know Type name conventionsIs suffixing type variables with I mean, a Type annotations for everything else?Your PR looks great so far, and thanks for adding type annotations to the PDAConfiguration and PDAStack classes. But if we're gonna embrace types in this library, every class/module should have type annotations. How easy would it be to add types to the remaining I guess I just ask because I've barely worked with types in Python and don't know that side of the stdlib very well, so you would probably be a lot faster than me at adding those annotations. But at the end of the day, I don't want to pressure you to do more than you're prefer to do. Other thingsI left you miscellaneous other comments in the PR, mostly small observations or questions I had around design desisions. Please review when you have time and let me know your thoughts! |
I'm actually not sure, from the other type annotations I've seen, there's not really a standard convention, I just do this sometimes to make it easier to tell what's a type and what's a class. But it's no trouble to change to/from this convention depending on what you prefer. Maybe there's a convention documented somewhere?
Definitely agree, I only stopped at the NFA/DFA because I ran out of time the day I was doing this but wanted to put up the work in progress as soon as possible.
Actually not hard, the most difficult files to annotate are the NFA/DFA because these are the longest.
I'm happy to! (and intend on doing so as part of this PR before merging). But if you'd like to take a stab at it, you're certainly welcome to. If you want to work with types some in Python, I actually think adding annotations to a library like this is a good way to learn. EDIT: Actually, I would encourage you to take a stab at some of this at least, since you'll need to set up typechecking as part of the CI job, and I think having some familiarity with the internals would be valuable 😃 |
|
@eliotwrobson Just a heads up—force-pushed to your branch a correction to the import order for the |
|
@caleb531 are you able to run |
@eliotwrobson No issues on my end, apparently (using mypy 1.1.1).
|
|
@caleb531 ahhh, I think I have a really old mypy version, that must be the issue. Forgot to mention in a previous comment: The next item related to this PR for me is going to be adding annotations to the regex library. Those are a little on the sophisticated side and might take me a while, so if you want to try your hand at annotating the other classes (TM, DTM, etc.) you're welcome to commit directly to my branch. I think that things will go much faster that way 😃 |
|
@eliotwrobson Yes, I'd be down for trying annotations for those other classes! It's starting to sound kinda fun, actually lol. My only ask is that you fix the conflict however you see fit. I noticed your |
|
@caleb531 Will go ahead and rebase, should be pretty simple. With respect to removing flake8-isort, here's a guide on how to set up black with other tools: https://black.readthedocs.io/en/stable/guides/using_black_with_other_tools.html Basically, you need to set some minor configuration options, but flake8 plays really nice with black if you set up the plugin. EDIT: Rebased, should be good to go! |
The Self type must be imported from typing_extensions, as it is only available natively in Python 3.11 otherwise.
Co-authored-by: Eliot Robson <eliot.robson24@gmail.com>
|
@caleb531 now that the annotations are getting more complete, I think it's a good opportunity to start setting up type checking tests. I'm not sure how this is typically done for packages (not many packages actually have inline type hints), but I think just running mypy before running tests should be enough on CI. To make the type stubs visible externally, I think we need to add a file as in this stack overflow post: https://stackoverflow.com/questions/62979389/is-there-a-best-practice-to-make-a-package-pep-561-compliant |
|
@eliotwrobson Hm, I was looking at flake8-mypy as a potential option (so that we can have all static code analysis managed through flake8), but it seems there may be some version compatibility issues (or may not be, one of us just needs to take a closer look). But if mypy needs to be run separately, I would prefer it be part of the I have a high-priority bug that I need to work on fixing for one of my other projects, so I don't have time to look into this at the moment. But let me know your thoughts, and I'll be happy to review anything you contribute further! |
|
@caleb531 no worries! At some point I'll try and take a look at some other libraries with integrated types and see how they're configured. Though It looks like flake8-mypy hasn't been maintained for some time. I'm not exactly sure what the preferred configuration is for modern projects. |
|
@caleb531 in the latest push, I've added a I didn't add type annotations to test cases, but I was wondering what else you'd like to see done before this gets merged. The changes here are pretty extensive, so I think it might make sense to do follow-up typing in the test files so that other branches don't get too out of sync with this one. |
|
@eliotwrobson I agree that I don't want this PR to sit for too long, especially given the other PRs we want to wrap up. I think we are in a place where we can merge this one safely, and then follow up with additional type PRs (e.g. for better mypy integration, types in tests, etc.). But as I see it, because mypy isn't currently complaining about the lack of types in tests, I think it's fine to prepare this PR for final review / merge. In other words, I don't have anything else for this PR. I am honestly ready for a final review / merge at this point. |
The Automaton superclass already defines the return type for validate(), and it should be inherited properly.
|
@caleb531 agreed. Since this is going into the develop branch I think it's fine to table some of the discussion items since they were pretty minor anyway. My only major item was a few missing type annotations but that can be handled in a followup. |
Work in progress branch to introduce type hints. Should probably wait until #121 and #129 get merged before doing the rest, since these will alter the type signatures that get used.
Also, need to add a mypy step to the CI workflow to check these annotations. Part of this PR (or at least the push to include type annotations) should be to run
black(or similar autoformatter) over the library. The function signatures with the annotations get pretty messy and it's annoying to have to fix these by hand.