Add missing type hints #197
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
--disallow-untyped-defs will warn if any function is missing type hints. These checks are currently skipped for tests/* because fixing codebasin/* should be the priority. Signed-off-by: John Pennycook <[email protected]>
Signed-off-by: John Pennycook <[email protected]>
Signed-off-by: John Pennycook <[email protected]>
Signed-off-by: John Pennycook <[email protected]>
Signed-off-by: John Pennycook <[email protected]>
Also fixes a bug where two calls to abspath and join had somehow been flipped. Signed-off-by: John Pennycook <[email protected]>
Signed-off-by: John Pennycook <[email protected]>
Signed-off-by: John Pennycook <[email protected]>
Adding the correct type hints here highlighted that there are two functions with surprising behavior; I've added FIXME and TODO comments to ensure we do not lose track of these. Signed-off-by: John Pennycook <[email protected]>
Adding type hints has helped to make the behavior of the file_source classes clearer, but also highlights how complicated they are. Signed-off-by: John Pennycook <[email protected]>
Signed-off-by: John Pennycook <[email protected]>
Fixing the type hints here required a small modification to one of the tests, because we were returning a list when we should have been returning a set. Signed-off-by: John Pennycook <[email protected]>
Adding these type hints highlighted a few places where the code is quite confusing, and I've added FIXME comments to remind us of those. There was one block of code that relied on dynamic typing to such an extent that I couldn't find a simple way to restructure it. I've added comments there to disable mypy for now, but we should come back and rewrite that function when we have a better idea of how it works. Signed-off-by: John Pennycook <[email protected]>
Pennycook
added
documentation
There was a problem hiding this comment.
Pull Request Overview
This PR configures mypy to check for missing type hints and adds comprehensive type annotations throughout the codebase to improve type safety and code quality.
- Configures mypy with
--disallow-untyped-defsto enforce type annotations - Adds type hints to almost all functions across the codebase
- Makes minimal code changes to satisfy mypy requirements
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| .pre-commit-config.yaml | Enables stricter mypy checking and excludes tests from type checking |
| tests/files/test_filetree_node.py | Updates test assertion to match change from list to set for platforms property |
| codebasin/tree.py | Adds return type annotations for CLI functions |
| codebasin/report.py | Comprehensive type hints for reporting functions and classes |
| codebasin/preprocessor.py | Extensive type annotations for preprocessor tokens, nodes, and parsing classes |
| codebasin/platform.py | Type hints for platform management functionality |
| codebasin/language.py | Simple type annotations for file language detection |
| codebasin/finder.py | Type hints for code analysis and parsing state management |
| codebasin/file_source.py | Type annotations for file source processing classes |
| codebasin/file_parser.py | Type hints for file parsing functionality |
| codebasin/config.py | Type annotations for configuration management |
| codebasin/_detail/logging.py | Type hints for logging utilities |
| codebasin/main.py | Type annotations for main entry point functions |
| codebasin/init.py | Type hints for core data structures |
Signed-off-by: John Pennycook <[email protected]>
|
I'm choosing to ignore the coverage failures here. A lot of the new lines are actually code that we expect to be unreachable, and designing a test for such cases seems impossible. Additionally, most (and possibly all) of the new lines are part of functionality that needs to be rewritten, and I'd rather focus on providing more robust tests (#96) to guide those efforts. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related issues
Proposed changes
mypyto check for missing type hints in addition to type errors.mypy.I'm sorry that this is so big, but I realized early on that doing this file by file didn't really work (even though the commit history suggests it did). Adding type hints to one file would often cause
mypyto go back and re-examine the code that called those functions, potentially introducing a bunch of new errors.But I think this was worth it, and the code is now much improved. There were quite a few places where adding the type information actually uncovered some bugs (or at least, assumptions) such as assuming that a function returns a value even though it's technically capable of returning
None. For the most part, I've satisfiedmypyby declaring which functions can sometimes returnNone, and then adding checks that they don't.In the long-term, I would like to revisit some of the problems that this change has highlighted, including:
| None, by preventing invalid look-ups entirely.file_source(see Rewrite file_source and file_parser #102) to avoid all the complexity related to the file-sourceCallables.Platforminto thePreprocessorso that we can use strong-typing without introducing a circular import.