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

Use return instead of twisted.defer.returnValue #2955

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

johannaengland
Copy link
Contributor

In the last week when running the tests I got a lot of a similar warning:

DeprecationWarning: twisted.internet.defer.returnValue was deprecated in Twisted 24.7.0; please use standard return statement instead
    defer.returnValue(serial)

I replaced all occurrences of defer.returnValue with return.
Reference: https://github.com/twisted/twisted/blob/HEAD/NEWS.rst#deprecations-and-removals, twisted/twisted#9930

returnValue has been deprecated in twisted 24.7.0
Copy link

codecov bot commented Aug 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.34%. Comparing base (ea42b94) to head (d959a80).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2955      +/-   ##
==========================================
- Coverage   56.35%   56.34%   -0.02%     
==========================================
  Files         603      603              
  Lines       43849    43838      -11     
  Branches       48       48              
==========================================
- Hits        24713    24702      -11     
  Misses      19124    19124              
  Partials       12       12              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@lunkwill42 lunkwill42 left a comment

Choose a reason for hiding this comment

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

While I'm really glad to see Twisted embrace newer Python constructs by deprecating their old kludges (that, in all fairness, were implemented to enable these kinds of programming paradigms under older Pythons), I think this approach should not be the immediate one.

Although the deprecation warnings do not actually break anything in NAV, they will be quite annoying, and so we have to make a decision about locking the Twisted version requirement.

Currently, NAV claims to work with at least version 23.8. If we make these changes, NAV will no longer work with Twisted 23, and we must upgrade to Twisted 24. It's either that, or lock to Twisted 23 in stable versions and make this change later.

I also think we should investigate the other changes mentioned in relation to this: The fact that the inlineCallbacks decorator might change or become deprecated in favor or Python's built-in co-routines. This would affect a lot of the ipdevpoll codebase as well.

There's also the slightly unrelated fact that a lot of ipdevpoll code was started even before inlineCallbacks appeared, and as such are purely callback-oriented. These parts of the codebase might stand to be refactored as well.

My dream is that we eventually will migrate the codebase to asyncio - the only reason we are sticking with Twisted is because of the inherited twistedsnmp interface of pynetsnmp. I believe it is quite possible to have Twisted and asyncio to co-exist these days. If the codebase could be mostly asyncio-oriented, with adaptations to keep pynetsnmp, that would be nice. There's also the option to rewrite pynetsnmp-2 to asyncio, since we've already forked it.

I.e. I want to keep this, but not merge it just yet.

@lunkwill42 lunkwill42 added blocked discussion Requires developer feedback/discussion before implementation labels Aug 20, 2024
@lunkwill42
Copy link
Member

Link for future reference: twisted/twisted#12239

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked deprecation discussion Requires developer feedback/discussion before implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants