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

[washort]

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.

[zomux]

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

[thijs]

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.

[zomux]

Fixed problems above by thijs :)

[thijs]

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.

[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.

[zomux]

fixed problems described by thijs.

[exarkun]

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.

[exarkun]

7. 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!

[thijs]

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

[thijs]

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

[tom.prince]

• 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.

[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.

[tom.prince]

 buildbot run

[exarkun]

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.

[tomprince]

(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.