Opened 3 years ago

Closed 21 months ago

#5544 task closed fixed (fixed)

Deprecate `twisted.python.util.unsignedID` and replace its uses with `id`

Reported by: washort Owned by: tom.prince
Priority: normal Milestone:
Component: core Keywords: easy
Cc: raphael@… Branch: branches/deprecate-unsignedid-5544
(diff, github, buildbot, log)
Author: thijs Launchpad Bug:

Description

twisted.python.util.unsignedID was created to deal with id returning negative numbers. Python 2.5 and later don't have this problem and id now returns a positive number always.

Replace all uses of this function in Twisted with id, then deprecate it.

Attachments (3)

5544.patch (13.2 KB) - added by zomux 3 years ago.
5544.fixed.patch (13.5 KB) - added by zomux 3 years ago.
5544.fixed2.patch (12.7 KB) - added by zomux 3 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 3 years ago by zomux

  • Owner set to zomux
  • Status changed from new to assigned

Changed 3 years ago by zomux

comment:2 Changed 3 years ago by zomux

  • Cc raphael@… added
  • Keywords review added
  • Owner zomux deleted
  • Status changed from assigned to new

Removed twisted.python.util.unsignedID , for deprecated after Python 2.5
Removed twisted.python.util.setIDFunction , the function used by unsignedID
Removed unit test test_unsignedID
Removed class UnsignedIDTests in twisted.python.test.test_util

comment:3 Changed 3 years ago by thijs

  • Keywords review removed
  • Owner set to zomux

Thanks for your patch.

  1. This change removes twisted.python.util.unsignedID but the compatibility policy states that it should be deprecated first
  2. twisted/topfiles/5544.removal should be one sentence.

Changed 3 years ago by zomux

comment:4 Changed 3 years ago by zomux

  • Keywords review added
  • Owner zomux deleted

Fixed problems above by thijs :)

comment:5 Changed 3 years ago by thijs

  • Keywords review removed
  • Owner set to zomux
  1. This patch changes the behaviour of the public setIDFunction in twisted/python/util.py but should continue to function as before but emit a deprecation warning instead. This also needs to be tested.
  2. The deprecation warning(s) should reference version 12.1.0 instead of 12.0.0
  3. the removal file doesn't flow correctly, what about:
    twisted.python.util.unsignedID is deprecated, use the built-in function "id" instead
    

Thanks.

comment:6 Changed 3 years ago by exarkun

Maybe rather than pointing out all the things wrong with each version of the patch, we should have some documentation about the right way to deprecate something. Getting to the right solution by random chance seems like it could take a while (and seems like a waste of time).

I created #5645 for documenting this and included at least a partial list of what to do when deprecating a function there.

Changed 3 years ago by zomux

comment:7 Changed 3 years ago by zomux

  • Keywords review added
  • Owner zomux deleted

fixed problems described by thijs.

comment:8 Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner set to zomux

Thanks.

  1. unsignedID is still imported in twisted/internet/tcp.py (as reported by pyflakes)
  2. in twisted/internet/test/test_base.py, the old assertion about the repr of a DelayedCall had one shortcoming and one advantage relative to the new assertion:
    1. It verified the address in the repr started with "0x". The new assertion relies on hex to return a string starting with this value, which I'd hesitate to rely on (new versions of Python may choose to change this behavior). Constructing the string using something like "0x%x" is probably better.
    2. The shortened hex string is inconsistent with builtin object reprs. "0x000000c8" would be a better output (given the old hard-coded id of 200). A subtlety with fixing this is selecting the right pointer size (32 bit vs 64 bit). It would be great if you could file a new ticket for fixing this.
  3. Same comment about the change to twisted/test/test_amp.py
  4. The unit test for unsignedID must not be deleted yet. The procedure for deprecating a function is to *preserve its behavior* apart from causing it to also emit a warning. This ensures that applications using the API continue to work. This is the purpose of the deprecation process: to avoid breaking applications based on affected APIs.
  5. Likewise, the implementation of unsignedID must therefore not be modified as this patch modifies it.
  6. Nor may unsignedID be removed from __all__ of its defining module.
  7. The change needs a "removal" news fragment, summarizing the deprecation and suggesting the replacement api (the builtin id method).

Thanks again.

comment:9 Changed 2 years ago by exarkun

  1. The change needs a "removal" news fragment, summarizing the deprecation and suggesting the replacement api (the builtin id method).

My mistake. You did add this news file, I wasn't paying close enough attention. It would be slightly improved by a recommendation to use id instead, though. Thanks again!

comment:10 Changed 21 months ago by thijs

  • Author set to thijs
  • Branch set to branches/deprecate-unsignedid-5544

(In [36833]) Branching to 'deprecate-unsignedid-5544'

comment:11 Changed 21 months ago by thijs

  • Keywords review added
  • Owner zomux deleted

Please review.

  1. added test for internet.tcp._BaseTCPClient.__repr__ that was using util.unsignedID
  2. removed usage of util.setIDFunction (was only used in tests)
  3. deprecated setIDFunction and unsignedID
  4. removed unused import from twisted.python.reflect

comment:12 Changed 21 months ago by tom.prince

  • Owner set to tom.prince

comment:13 Changed 21 months ago by tom.prince

  • Keywords review removed
  • The tests for the deprecated functions should suppress the warnings from those functions, using something like
    class Tests(unittest.TestCase):
        suppress  = [((), {'action': ignore', 'message':'%s was deprecated' % (reflect.fullyQualifiedName(unsignedId),), 'category':DeprecationWarning})]
    
  • There is still mixed use of hex and '0x%x' to format ids. A single one should be used consistently.

Please commit after fixing the above.

  • Since twisted.test.connectionmixins.TCPClientTestsMixin.test_addresses is being changed, it would be nice to make a single assert with all the relevant data.

comment:14 Changed 21 months ago by exarkun

The tests for the deprecated functions should suppress the warnings from those functions, using something like

Yes, except actually use twisted.trial.util.suppress instead.

comment:15 Changed 21 months ago by tom.prince

  • Keywords review added

comment:16 Changed 21 months ago by tom.prince

  • Owner tom.prince deleted

comment:17 Changed 21 months ago by exarkun

  • Owner set to exarkun
  • Status changed from new to assigned

comment:18 Changed 21 months ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to tom.prince
  • Status changed from assigned to new
  1. I don't like the change from import a, b to import a\nimport b\n. This is something PEP8 talks about, I guess, but it's a point where Twisted's style diverges. I don't see any readability reasons to limit an import statement to a single name (we certainly don't come anywhere close to adhering to that for all of our twisted imports). I don't really care if this branch changes a few imports from one style to the other, but I hope you (alas, I have no idea who "you" is, as this branch has been touched by so many) don't go out of your way to make this change in the future. Change it back if you like, otherwise just keep it in mind for the future.
  2. test_defer module docstring is missing an article - "the" would fit nicely before the module name. Alternatively, drop the trailing module.
  3. The change to the test in twisted/internet/test/test_base.py makes the assertion worse. Previously the test was very clear about what string it expected. Now it relies on "%x" being the correct way to format an integer in hex to construct the string it expects. String literals are better than constructed strings in this context. They leave less room for mistakes and misunderstandings. That said, since there is no setBuiltinID and since monkey patching the id attribute of the base module is pretty gross, maybe this ticket inevitably just makes this test (and others) worse.
  4. in test_reflectpy3, test_classIDStr seems misnamed to me. The test exercises safe_repr. I also think the point of the test has nothing to do with class objects, really; it's about the inclusion of the (arbitrary) object id in the error report string.
  5. The actual assertion made by that test makes is pretty weak as well, assertIn tells us very little about the string (clearly that's a pre-existing issue, not a problem introduced by this change).
  6. The indentation for the new code in test_addresses is all a bit funny. This is the sort of thing I really wish twistedchecker knew about. :/

Please fix 2, 4, and 6 and file a ticket for 5, then merge. Thanks.

comment:19 Changed 21 months ago by tomprince

  • Resolution set to fixed
  • Status changed from new to closed

(In [36856]) Merge deprecate-unsignedid-5544: Deprecate twisted.python.util.unsignedID and replace its uses with id.

Author: zomux, thijs, tom.prince
Reviewers: tom.prince, exarkun
Fixes: #5544

twisted.python.util.unsignedID was created to deal with id returning negative
numbers. Python 2.5 and later don't have this problem and id now returns a
positive number always.

Replace all uses of this function in Twisted with id, then deprecate it.

Note: See TracTickets for help on using tickets.