Updates round 2 #2329
Updates round 2 #2329
Conversation
This package is in maintenance only mode and only had two uses. Replace their use with the `tmp_path` fixture as suggested in the docs[1] Link: https://docs.pytest.org/en/8.3.x/reference/reference.html#tmpdir [1]
There was one new error:
TypeError: exceptions must be derived from Warning, not <class 'NoneType'>
Fix this following the docs[1]
Link: https://docs.pytest.org/en/8.3.x/how-to/capture-warnings.html#additional-use-cases-of-warnings-in-tests [1]
This required fixes an error in the config (I also updated the relevant
link, the old one would 404):
ValueError: Error code '#' supplied to 'extend-ignore' option does not match '^[A-Z]{1,3}[0-9]{0,3}$'
And some errors in the code:
isort/literal.py:59:8: E721 do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()`
tests/unit/test_isort.py:13:1: F401 'typing.Dict' imported but unused
The second test conflicted with `mypy`, since `Dict` was only used in
commented annotations, to make them both happy move one of these
annotations out of a comment (in the future we should probably move all
such annotations out of comments)
This required addressing some errors from `mypy`:
tests/integration/test_setting_combinations.py:20: error: "SearchStrategy" expects no type arguments, but 1 given [type-arg]
tests/integration/test_hypothesmith.py:36: error: "SearchStrategy" expects no type arguments, but 1 given [type-arg]
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2329 +/- ##
=======================================
Coverage 99.12% 99.12%
=======================================
Files 39 39
Lines 3095 3095
Branches 787 787
=======================================
Hits 3068 3068
Misses 15 15
Partials 12 12 |
tests/unit/test_main.py
Outdated
| python_file.write(import_content) | ||
| tmpdir.join("no_imports.py").write("...") | ||
| subprocess.run(["git", "init", str(tmp_path)]) | ||
| python_file = tmp_path.joinpath("has_imports.py") |
There was a problem hiding this comment.
Could you use / here as well as other places? I believe that is more idiomatic?
tests/unit/test_main.py
Outdated
| should_check = ["/git_project0/has_imports.py"] | ||
|
|
||
| assert all(f"{str(tmpdir)}{file}" in error for file in should_check) | ||
| assert all(f"{str(tmp_path)}{file}" in error for file in should_check) |
There was a problem hiding this comment.
Using both str and f" shouldn't be necessary for Path
There was a problem hiding this comment.
Using both
strandf"shouldn't be necessary forPath
what do you think about making this a separate change: I see there are several points in this file (including parts not touched by this change) that could do with the same update? I'm happy to include it in this patch, but my main goal was to just get things working, without changing things too much
There was a problem hiding this comment.
Is it possible to exclude the py change and do that together with the changes in a separate PR?
If not, let's at least make sure the code we're touching now in this diff is correct and worry about the rest later.
There was a problem hiding this comment.
If not, let's at least make sure the code we're touching now in this diff is correct and worry about the rest later.
👍 830eca7
tests/unit/test_isort.py
Outdated
|
|
||
| # test again with a custom configuration | ||
| custom_configuration = { | ||
| custom_configuration: Dict[str, Any] = { |
There was a problem hiding this comment.
Can you use dict[str, Any]?
There was a problem hiding this comment.
Can you use
dict[str, Any]?
yep, now that we don't support Python 3.8 that will work
|
I've pushed some fixup commits, so let me know just before merge and I'll rebase to squash them back in to the correct commits |
|
Lint is unhappy
becuase that file uses type comments like EDIT: actually, it complains even when imported only when EDIT EDIT: 4efc999 |
|
See PyCQA/meta#64 (comment) I have stopped merging anything. |
staticdev
left a comment
staticdev
left a comment
There was a problem hiding this comment.
LGTM, thanks a lot @matthewhughes934.
3 participants
Second round of updates, split out to keep PRs reasonable size
Drop
pydependencyThis package is in maintenance only mode and only had two uses. Replace
their use with the
tmp_pathfixture as suggested in the docs[1]Link: https://docs.pytest.org/en/8.3.x/reference/reference.html#tmpdir [1]
Update
pytestThere was one new error:
Fix this following the docs[1]
Link: https://docs.pytest.org/en/8.3.x/how-to/capture-warnings.html#additional-use-cases-of-warnings-in-tests [1]
Update
blackUpdate
flake8This required fixes an error in the config (I also updated the relevant
link, the old one would 404):
And some errors in the code:
The second test conflicted with
mypy, sinceDictwas only used incommented annotations, to make them both happy move one of these
annotations out of a comment (in the future we should probably move all
such annotations out of comments)
Update
hypothesisThis required addressing some errors from
mypy: