Opened 5 years ago

Closed 5 years ago

#6342 enhancement closed fixed (fixed)

Deprecate twisted.python.hashlib

Reported by: Thijs Triemstra Owned by: Thijs Triemstra
Priority: normal Milestone:
Component: core Keywords: easy
Cc: Thijs Triemstra Branch: branches/deprecate-hashlib-6342
branch-diff, diff-cov, branch-cov, buildbot
Author: rodney757

Description

hashlib was introduced in py2.5 and twisted.python.hashlib was added in 2007 (#2763) to "allow[s] application code to transparently use APIs which existed before C{hashlib} was introduced or to use C{hashlib} if it is available."

Twisted supports Python 2.6 and up and twisted's hashlib can be deprecated, and uses of it replaced, to make it easier to support py3 and discourage users from using this module.

Initially I wanted to file a ticket for porting this module to py3 but why port it when its there for (now) unsupported versions of Python.

Attachments (3)

ticket6342.patch (18.3 KB) - added by rodney757 5 years ago.
ticket6342.2.patch (19.0 KB) - added by rodney757 5 years ago.
lastest patch
ticket6342.3.patch (19.1 KB) - added by rodney757 5 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 5 years ago by Itamar Turner-Trauring

Keywords: easy added

You can see an example deprecation in twisted/conch/insults/__init__.py and the applicable tests in twisted/conch/test/test_insults.py.

Changed 5 years ago by rodney757

Attachment: ticket6342.patch added

comment:2 Changed 5 years ago by rodney757

Keywords: review added
Owner: set to rodney757
Status: newassigned

comment:3 Changed 5 years ago by rodney757

Status: assignednew

comment:4 Changed 5 years ago by rodney757

Owner: rodney757 deleted

comment:5 Changed 5 years ago by Tom Prince

Keywords: review removed
Owner: set to rodney757

Thanks for your contribution.

  1. Don't delete the existing test. Even if people shouldn't be using the code, we don't want the code to break. The tests should only be removed when the code they are testing gets removed.
    • You'll want to make sure that the warning is suppressed in those tests (using the .suppress attribute and twisted.trial.util.suppress).
  2. Similarily, the module docstring doesn't want to be completely deleted. On the other hand, it should be changed to include the deprecation information (twisted.python.deprecate._getDeprecationDocstring has the standard format for that). It probably also wants to be changed to indicate that twisted doesn't use it anymore.
  3. There are still a couple of uses t.p.hashlib left after applying your patch. - `twisted/conch/client/knownhosts.py'
    • twisted/conch/ssh/keys.py
    • twisted/web/server.py
  4. Your change needs a news file.
  5. The conditional (try) logic in t.p.hashlib can be removed. On supported versions, the except will never trigger.
  6. Refer to 'stdlib' or 'standard library' not 'Python Core'.

Changed 5 years ago by rodney757

Attachment: ticket6342.2.patch added

lastest patch

comment:6 Changed 5 years ago by rodney757

Keywords: review added
Owner: rodney757 deleted

comment:7 Changed 5 years ago by Tom Prince

Keywords: review removed
Owner: set to rodney757

Thanks for your contribution.

  1. You can probably just do something like
    from hashlib import md5, sha1
    
    The code was structured the way it was before so it only did the import once. (Although, on further consideration, I'm not sure why the code wasn't like that to begin with.
  2. 13.0.0 was the most recent release, so 13.1.0 will be the next release. That will be the release where this will be deprecated.
  3. Calls to .flushWarnings should include a function to blame. This is to verify that the stacklevel argument is set correctly. It should point to the function that is invoking the deprecated functionality.
  4. The spacing between functions and classes doesn't follow the coding standard.
    • (minor) Also, there typically isn't a blank line after method docstrings
    • There is a spurious blank line at the begining of hashlib's module docstring.

Changed 5 years ago by rodney757

Attachment: ticket6342.3.patch added

comment:8 Changed 5 years ago by rodney757

Keywords: review added
Owner: rodney757 deleted

comment:9 Changed 5 years ago by Thijs Triemstra

Author: thijs
Branch: branches/deprecate-hashlib-6342

(In [38141]) Branching to 'deprecate-hashlib-6342'

comment:10 Changed 5 years ago by Thijs Triemstra

(In [38145]) correct version number, refs #6342

comment:11 Changed 5 years ago by Thijs Triemstra

Thanks for your patch. The version nr was still incorrect so I fixed that and forced a build.

Leaving the review keyword so someone else can review (and/or merge) this since I made some minor changes to the patch (inc adding news files).

comment:12 Changed 5 years ago by Richard Wall

Owner: set to Richard Wall
Status: newassigned

Reviewing...

comment:13 Changed 5 years ago by Richard Wall

Keywords: review removed
Owner: changed from Richard Wall to Thijs Triemstra
Status: assignednew

Code Review

Thanks for the branch. Here's a quick review.

Notes:

  • Branch merges cleanly to trunk
  • Tests pass locally on Fedora 18 x86_64

I spotted one minor thing:

  1. source:branches/deprecate-hashlib-6342/twisted/python/test/test_hashlib.py
    1. test_deprecation
      1. "Test to" in docstring is unnecessary

Otherwise the branch looks fine and I couldn't find any remaining uses of twisted.python.hashlib. Fix that docstring and then merge.

-RichardW.

comment:14 Changed 5 years ago by Thijs Triemstra

Author: thijsrodney757
Status: newassigned

Thanks for the review, I'll fix that docstring and merge.

comment:15 Changed 5 years ago by Thijs Triemstra

(In [38409]) address review comment, refs #6342

comment:16 Changed 5 years ago by Thijs Triemstra

Resolution: fixed
Status: assignedclosed

(In [38410]) Merge deprecate-hashlib-6342: Replace usage of twisted.python.hashlib and deprecate it in favor of the hashlib module in the standard library.

Author: rodney757 Reviewer: tom.prince, thijs, rwall Fixes: #6342

Note: See TracTickets for help on using tickets.