Opened 4 years ago

Closed 3 years ago

#6342 enhancement closed fixed (fixed)

Deprecate twisted.python.hashlib

Reported by: thijs Owned by: thijs
Priority: normal Milestone:
Component: core Keywords: easy
Cc: thijs 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 3 years ago.
ticket6342.2.patch (19.0 KB) - added by rodney757 3 years ago.
lastest patch
ticket6342.3.patch (19.1 KB) - added by rodney757 3 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 3 years ago by itamar

  • 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 3 years ago by rodney757

comment:2 Changed 3 years ago by rodney757

  • Keywords review added
  • Owner set to rodney757
  • Status changed from new to assigned

comment:3 Changed 3 years ago by rodney757

  • Status changed from assigned to new

comment:4 Changed 3 years ago by rodney757

  • Owner rodney757 deleted

comment:5 Changed 3 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 3 years ago by rodney757

lastest patch

comment:6 Changed 3 years ago by rodney757

  • Keywords review added
  • Owner rodney757 deleted

comment:7 Changed 3 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 3 years ago by rodney757

comment:8 Changed 3 years ago by rodney757

  • Keywords review added
  • Owner rodney757 deleted

comment:9 Changed 3 years ago by thijs

  • Author set to thijs
  • Branch set to branches/deprecate-hashlib-6342

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

comment:10 Changed 3 years ago by thijs

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

comment:11 Changed 3 years ago by thijs

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 3 years ago by rwall

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

Reviewing...

comment:13 Changed 3 years ago by rwall

  • Keywords review removed
  • Owner changed from rwall to thijs
  • Status changed from assigned to new

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 3 years ago by thijs

  • Author changed from thijs to rodney757
  • Status changed from new to assigned

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

comment:15 Changed 3 years ago by thijs

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

comment:16 Changed 3 years ago by thijs

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

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