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

Miscellaneous type-checking updates #2614

Merged
merged 3 commits into from
Aug 31, 2024
Merged

Miscellaneous type-checking updates #2614

merged 3 commits into from
Aug 31, 2024

Conversation

dgw
Copy link
Member

@dgw dgw commented Aug 13, 2024

Description

types-pkg-resources was yanked, which I discovered when submitting #2613. pkg_resources itself is typed in setuptools>=71.1, negating the need to install type stubs.

When testing that fix, I updated mypy and friends (pip install -U -r dev-requirements.txt) and found a few more new type-check errors caught by the newer tools:

  • SopelIdentifierMemory.update()'s signature isn't compatible with its supertype (fixed by adding typing_extensions.override decorator; can't go stdlib-only until py3.12…)
  • tools.OutputRedirect calls methods on sys.__stdout__ and sys.__stderr__, which are allowed to be None and therefore have to be guarded with ifs (I'm glad this class is deprecated)

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make lint and make test)
  • I have tested the functionality of the things this change touches

Notes

All of this needs backporting to the 8.0.x maintenance branch, which is why this has the 8.0.1 milestone.

dgw added 3 commits August 12, 2024 20:10
`pkg_resources` itself is typed in setuptools>=71.1, negating the need
to install type stubs.
mypy has to be told this explicitly, which means a new "runtime"
dependency/import that only has to do with type-checking... because
decorator evaluation can't be deferred like annotations can.

Sigh.
I'm very glad this is deprecated. The only reason I'm bothering to fix
this is because we can't remove it from the 8.0.x maintenance branch.
@dgw dgw added High Priority Housekeeping Code cleanup, removal of deprecated stuff, etc. labels Aug 13, 2024
@dgw dgw added this to the 8.0.1 milestone Aug 13, 2024
@dgw dgw requested a review from a team August 13, 2024 03:41
@dgw
Copy link
Member Author

dgw commented Aug 13, 2024

Manually re-running CI jobs got them to pass, but I do not like the random failure in 3.9's sopel/builtins/rand.py::test_example_rand_4 test that canceled the remaining unfinished jobs. I believe this error should be impossible, but it happened somehow and probably needs its own investigation (😩):

=================================== FAILURES ===================================
_____________________________ test_example_rand_4 ______________________________
sopel/tests/pytest_plugin.py:137: in test
    assert len(outputs) == expected_output_count
E   AssertionError: assert 9 == 10
E    +  where 9 = len(['random(0, 2): 1', 'random(0, 2): 1', 'random(0, 2): 1', 'random(0, 2): 2', 'random(0, 2): 1', 'random(0, 2): 1', ...])

Based on the output, test_example_rand_4 is this example in sopel/builtins/rand.py:

@plugin.example('.rand 2', r'random\(0, 2\): (0|1|2)', re=True, repeat=10)

How could repeat=10 somehow yield only 9 outputs? OTFGK??

Copy link
Contributor

@Exirel Exirel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah all good, I'm glad we have the mypy check in the CI.

And also this:

removed_in='8.1'

Can't wait for that!

dgw added a commit that referenced this pull request Aug 31, 2024
@dgw dgw merged commit 1c6aeb9 into master Aug 31, 2024
15 checks passed
@dgw dgw deleted the type-check-updates branch August 31, 2024 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
High Priority Housekeeping Code cleanup, removal of deprecated stuff, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants