Introduce mypy (gradually) #265
Conversation
Codecov Report
@@ Coverage Diff @@
## master #265 +/- ##
==========================================
- Coverage 84.21% 84.12% -0.09%
==========================================
Files 21 21
Lines 1755 1758 +3
Branches 271 271
==========================================
+ Hits 1478 1479 +1
- Misses 233 235 +2
Partials 44 44
Continue to review full report at Codecov.
|
ward/_diff.py
Outdated
| if isinstance(output_lines[-1], str): | ||
| line_to_rewrite = output_lines[-1] | ||
| else: | ||
| line_to_rewrite = output_lines[-1].plain | ||
| output_lines[-1] = self.rewrite_line(line, line_to_rewrite, prev_marker) | ||
| else: | ||
| output_lines.append(line[2:]) |
There was a problem hiding this comment.
This I'm not 100% sure about. Maybe line 99 (before changes) should instead append a Text object instead of str?
There was a problem hiding this comment.
I think we should try to avoid Union where possible, so always appending Text (with no args other than the string) instead of str would make sense. They'll be treated the same by Rich anyway.
There was a problem hiding this comment.
That makes tests fail however, so treatment is not 100% the same 😄
Maybe this should be improved upon in a follow-up?
There was a problem hiding this comment.
I'll take a look at the tests and push a fix to this branch (assuming it's a simple fix, which I think it is)
There was a problem hiding this comment.
I've pushed up a change to always use Text and deleted the if isinstance block that was added. Let me know if there's any issues with that
There was a problem hiding this comment.
Looks great, this is also my preferred solution as long as tests pass!
| @@ -129,7 +129,7 @@ def teardown(self): | |||
| asyncio.get_event_loop().run_until_complete(awaitable) | |||
|
|
|||
|
|
|||
| def fixture(func=None, *, scope: Optional[Union[Scope, str]] = Scope.Test): | |||
| def fixture(func=None, *, scope: Union[Scope, str] = Scope.Test): | |||
There was a problem hiding this comment.
Here I'm also a bit unsure. Is there a good reason that None should be allowed as input? If yes, then it should probably be handled before Scope.from_str(scope).
There was a problem hiding this comment.
This shouldn't be Optional -- your change here is correct :)
| @@ -54,7 +55,7 @@ def _get_debugger_hook(hookname: str): | |||
| modname, dot, funcname = hookname.rpartition(".") | |||
| if dot == "": | |||
| modname = "builtins" | |||
| module = importlib.import_module(modname) | |||
| module: Any = importlib.import_module(modname) | |||
There was a problem hiding this comment.
I think this can be types.ModuleType
There was a problem hiding this comment.
I think line 60 will be a problem then (accessing Pdb attribute of import pdb. That said, maybe a type: ignore on line 60 is better than this. Or equally good/bad, I don't have a preference really.
There was a problem hiding this comment.
Ah, I see. You could possibly create a Protocol to handle this, but in all honesty it's hardly worth the effort.
There was a problem hiding this comment.
Yep and the Protocol then doesn't make much sense in the else clause where hookname != "pdb.set_trace".
I think this is just dynamically typed code and should be ignored by static type checker.
|
From my side, this PR can already be considered ready to merge, with the idea that follow-up PRs remove the module ignoring lines from pyproject.toml and fix any type checker errors in the respective modules. Another strategy would be to make this a mega PR that fixes everything (a slightly worse strategy if you ask me but will also work 😄 ) |
|
I agree with the approach of doing this gradually. I'll merge this in now, thanks! :) |
* master: (133 commits) Prepare 0.63.0b0 Distribute type data (PEP 561) (darrenburns#283) Type check `ward.testing` (darrenburns#282) [pre-commit.ci] pre-commit autoupdate (darrenburns#280) Add pretty output for all comparison failures (darrenburns#256) Update conf.py Update pyproject.toml Make sure raises raises assertion error when no exception is raised (darrenburns#281) [pre-commit.ci] pre-commit autoupdate (darrenburns#275) Fix mypy error Prepare 0.62.0b0 Update writing_tests.rst Allow subclasses of specified exception class to pass raises assertion (darrenburns#279) Type check `ward.expect`, `ward._fixtures` and `ward._terminal` (darrenburns#274) Only require `poetry-core` as build system (darrenburns#277) Prepare 0.61.1b0 Allow Click 7 (darrenburns#272) Type check `ward._config` (darrenburns#269) Correct plural, singular forms darrenburns#244 (darrenburns#258) Prepare 0.61.0b0 Switch from `toml` to `tomli` for TOML v1 compat (darrenburns#267) Introduce mypy (gradually) (darrenburns#265) Don't update the lockfile with 'make prep' (darrenburns#266) Let Poetry automatically update license and py version classifiers (darrenburns#260) Prepare 0.60.1b0 Fix broken command (darrenburns#257) fix typo (darrenburns#255) Prepare 0.60.0b0 Update live output gif for docs Add info on `live` output mode to docs Print pretty failure messages for `in` and `not in` (darrenburns#242) Add `live` progress style and refactor output and progress styles (darrenburns#233) only get test source once (darrenburns#249) [pre-commit.ci] pre-commit autoupdate (darrenburns#248) pygments dependency removed - no longer required Update build.yml Update README.md Prepare 0.59.0b0 Use Rich for displaying diffs in errors (darrenburns#235) Prepare 0.58.0b0 Several bug fixes (darrenburns#241) combine build and pullrequest workflows (darrenburns#236) Add Python 3.10 Beta support to CI (darrenburns#230) Update CONTRIBUTING.md Prepare 0.57.2b0 Remove comment from build pipeline Bump snok/install-poetry from 1.1.1 to 1.1.6 (darrenburns#232) add dependabot.yml for gha version bumps (darrenburns#231) Add pre-commit (darrenburns#222) fix linter errors (darrenburns#217) ...
This PR fixes very few type errors, but adds a working configuration. Please see the mypy configuration in pyproject.toml: most of the codebase is still ignored by mypy, but this is a good base to start building on I think.