tools, tools.calculation: docs/type-hint improvements, API fixes, better test coverage #2543
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
I forgot to put `.. deprecated:: 8.0` somewhere when deprecating the `OutputRedirect` class, last time I took a swing at this module.
Sopelunked into the history and found when all this stuff was originally added to the codebase. (Spoiler: A long time ago.) There was no docstring at all for `EquationEvaluator`, so I wrote one. Tweaked the docs page for `tools.calculation` to show everything instead of only what's listed in `__all__`, and noted that most of the module is considered internal machinery as we've been doing in `irc` etc. NOTE: I would like the "public function" to appear at the top, but could not find a nice way to do that in ~15 min of googling+experimenting.
Everything in this module can be considered "added" at the same time as the module itself, in v5.3. Even going back that far crosses a rename boundary (from `willie` to `sopel`) and it's likely a fool's errand to try and write a plugin that functions equally well under Sopel 8.0-dev, 7.x, and everything else back to 4.x. In the immortal words of Lisa Landry, "Let it go, Ray." (But this "undo" operation is kept as a separate commit in case other maintainers feel differently.)
dgw
added
Documentation
Bugfix
SnoopJ
approved these changes
Nov 3, 2023
Contributor
SnoopJ
left a comment
SnoopJ
left a comment
There was a problem hiding this comment.
LGTM, some fine Sopelunking here! 100% coverage train, choo choo! 🚂💨
dgw
force-pushed
the
tools.calculation-typing
branch
from
bbaafc6 to
3f09032
Compare
Contrary to its parameter descriptions, `pow_complexity()` was NOT able to handle `float` inputs. The `&` operator does not work with floats, nor do floats have a `bit_length()` method. `EquationEvaluator.__call__()` now takes an optional `timeout` argument just like its upstream counterpart in `ExpressionEvaluator`.
Co-authored-by: dgw <[email protected]>
Nasty assertions on exception args... Really need to refactor that silly ExpressionEvaluator.Error type into some sensible PUBLIC hierarchy.
dgw
force-pushed
the
tools.calculation-typing
branch
from
3f09032 to
3c4e477
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
This starts with some tweaks to the docstring data in
tools(.. versionadded::kinda stuff) andtools.calculationthat I found sitting in a forgotten branch; apparently those smaller tweaks never reached the point of a full PR.That stalled work's finally done:
sopel/tools/calculation.pyis now fully type-annotated and has 100% test coverage. Along the way I discovered a few API glitches (e.g. incorrectly documented parameter types, missing parameters) and fixed 'em.I admit some of the test assertions are pretty ugly, but with only one custom exception type it's necessary to make sure that the right kind of
ExpressionEvaluator.Erroris being raised. A future refactoring opportunity, that is (which #2508 already tracks).Checklist
make qa(runsmake lintandmake test)Notes
The framework for fully exercising
tools.calculationin tests came from both #2530 and #2464 (comment) — thanks, @SnoopJ! Getting around to this would have taken a lot longer if you didn't kick-start it.Just one thing I haven't cracked: Two new Sphinx warnings from type annotations that it can't resolve to cross-reference links:
Those are, however, the types required to satisfy
mypy. Trying to use the CamelCase classes inast's documentation (BinOpandUnaryOp) never passed type-checking. The class hierarchy there doesn't work like one might think it does (or should).At the moment, I'm ready to give up on these like what eventually happened with the unfixed warnings left over in #2496. Really seems like Sphinx just can't handle certain things. Even the Python docs themselves reference these, and the rendered HTML shows them as
xref py py-classbut they're not linked.