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

#12026 Adjust to deprecation of 3-arg generator.throw() #12027

Merged
merged 1 commit into from
Dec 29, 2023

Conversation

AdamWill
Copy link
Contributor

In Python 3.12, the 3-arg signature of generator.throw() is deprecated, you're only supposed to use the 1-arg signature where you pass only an exception instance. I think this is the right thing to pass in this case.

Scope and purpose

Fixes #12026 (use of a deprecated form of generator.throw() causes deprecation warnings on Python 3.12). Note we are not totally sure yet whether this is safe on Python 3.8: see here.

Contributor Checklist: (done)

@AdamWill
Copy link
Contributor Author

gah. stupid comment cleanup. just a sec.

@AdamWill AdamWill changed the title In Python 3.12, the 3-arg signature of generator.throw() is #12026 Adjust to deprecation of 3-arg generator.throw() Oct 28, 2023
@AdamWill
Copy link
Contributor Author

#12028 is an alternative that just suppresses the warning.

@AdamWill
Copy link
Contributor Author

oh, note I think this is the right thing to pass, but possibly it should just be self.value - I wasn't 100% sure.

Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

Changes looks good.

There were some CI failures, but I don't think they are related.

Retrying the CI.

It would be nice to have an automated test, to make sure we don't regress.

Thanks for the PR.

@@ -0,0 +1 @@
adjust generator `throw()` call to avoid triggering deprecation warnings on Python 3.12
Copy link
Member

Choose a reason for hiding this comment

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

Maybe something like this ?

Suggested change
adjust generator `throw()` call to avoid triggering deprecation warnings on Python 3.12
twisted.python.failure.Failure now throws exception for generators without triggering a deprecation warnings on Python 3.12.

@@ -516,7 +516,7 @@ def throwExceptionIntoGenerator(self, g):
"""
# Note that the actual magic to find the traceback information
# is done in _findFailure.
return g.throw(self.type, self.value, self.tb)
return g.throw(self.value.with_traceback(self.tb))
Copy link
Member

Choose a reason for hiding this comment

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

We were already using this code in CI with Python 3.12

I guess that the warning was raised, but there was no check for it.

It would be nice to have an explicit test that no warnings are raised, on any Python version.

@adiroiban
Copy link
Member

@AdamWill based on the comment from #12028 does it mean that this PR lacks sufficient test coverage?

I can see that it uses the single argument version and the tests pass on 3.8 and I guess that this code is exercise in the python 3.8

Do we need #12028?

Regards

@AdamWill
Copy link
Contributor Author

well, I'm not an expert on twisted, just a distro guy who noticed this problem, so I don't know for sure how extensive the test suite is. if you're happy with the test suite's coverage, then I guess this one should be fine.

@glyph
Copy link
Member

glyph commented Oct 30, 2023

well, I'm not an expert on twisted, just a distro guy who noticed this problem, so I don't know for sure how extensive the test suite is. if you're happy with the test suite's coverage, then I guess this one should be fine.

The suite is unfortunately kinda hit-and-miss in some areas, but in core abstractions like Deferred it is fairly exhaustive. I'd be confident that if the tests are passing for this, then it probably works fine.

Given how close to the core this change is, it might not be amiss to ping the mailing list and see if anyone could run their own test suites, just to be sure we haven't missed something before the next release. But that's really just belt-and-suspenders checking.

@adiroiban
Copy link
Member

Email sent https://mail.python.org/archives/list/twisted@python.org/thread/MJFZT7AC4CTA54U5KNSWP2CZQDBIFQZK/

We can wait 1 week and after that we can merge this.

Thanks Adam and Glyph for your feedback

@glyph
Copy link
Member

glyph commented Dec 26, 2023

We can wait 1 week and after that we can merge this.

I didn't see any complaints, shall we merge?

@adiroiban
Copy link
Member

My only minor complain about the merge is the text if the release notes.
The current text reads more like a commit message, rather than a public release notes.

In Python 3.12, the 3-arg signature of generator.throw() is
deprecated, you're only supposed to use the 1-arg signature
where you pass only an exception instance. I *think* this is the
right thing to pass in this case.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
@AdamWill
Copy link
Contributor Author

Rebased and updated the changelog text as requested by @adiroiban .

@adiroiban adiroiban merged commit 88151eb into twisted:trunk Dec 29, 2023
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

twisted uses deprecated three-argument form of generator.throw
4 participants