-
-
Notifications
You must be signed in to change notification settings - Fork 25
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
Support strict_exception_groups=True #188
base: master
Are you sure you want to change the base?
Changes from all commits
8f04a5c
7eb779b
c3f7dc2
8fd6db1
69853c3
f22fa75
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,5 +4,4 @@ pip-tools>=5.5.0 | |
pytest>=4.6 | ||
pytest-cov | ||
pytest-trio>=0.5.0 | ||
trio<0.25 | ||
trustme |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
# requirements for `make lint/docs/publish` | ||
mypy | ||
pylint | ||
mypy | ||
sphinx | ||
sphinxcontrib-trio | ||
sphinx_rtd_theme | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi John-- I may be slow in reviewing. Please make sure the PR description summarizes the changes (e.g. mentions the internal error, API policy about exception groups, etc.). Would need test coverage for any changes, but ok to hold off until I've reviewed the basics (as I can't promise we'll take the direction of this PR).
Please see #177. On type checking, I'm ok running a loose mypy pass, but reluctant to convert the codebase to strict type annotations, since fighting the type system is often a burden on maintenance (especially with a project on life support). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah thank you, of course I'd complained about lack of Black before 😅 Agree on loose mypy just to check if the annotations that exist are correct. And I don't have any plans on adding annotations everywhere, just doing it for my own sake when developing. Will try and make PR summary verbose and descriptive 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In principle I'd like to state that the API doesn't raise exception groups, and that would include the server API. For the user's handler, perhaps add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that for users that are embracing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for cases like this, I would rather see an explicit
assert exc
in the code over a pylint annotationThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's
BaseExceptionGroup
that pylint fails to realize will always be available. But can switch to anassert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...except mypy correctly picks up that it will always be truthy, and raises a warning if doing
assert BaseExceptionGroup
>.>