Opened 9 years ago
Closed 9 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)
Change History (19)
comment:1 Changed 9 years ago by
Keywords: | easy added |
---|
Changed 9 years ago by
Attachment: | ticket6342.patch added |
---|
comment:2 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | set to rodney757 |
Status: | new → assigned |
comment:3 Changed 9 years ago by
Status: | assigned → new |
---|
comment:4 Changed 9 years ago by
Owner: | rodney757 deleted |
---|
comment:5 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to rodney757 |
Thanks for your contribution.
- 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 andtwisted.trial.util.suppress
).
- You'll want to make sure that the warning is suppressed in those tests (using the
- 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. - 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
- Your change needs a news file.
- The conditional (try) logic in
t.p.hashlib
can be removed. On supported versions, theexcept
will never trigger. - Refer to 'stdlib' or 'standard library' not 'Python Core'.
comment:6 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | rodney757 deleted |
comment:7 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to rodney757 |
Thanks for your contribution.
- You can probably just do something like
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.
from hashlib import md5, sha1
- 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.
- 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. - 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 9 years ago by
Attachment: | ticket6342.3.patch added |
---|
comment:8 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | rodney757 deleted |
comment:9 Changed 9 years ago by
Author: | → thijs |
---|---|
Branch: | → branches/deprecate-hashlib-6342 |
(In [38141]) Branching to 'deprecate-hashlib-6342'
comment:11 Changed 9 years ago by
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:13 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Richard Wall to Thijs Triemstra |
Status: | assigned → 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:
- source:branches/deprecate-hashlib-6342/twisted/python/test/test_hashlib.py
- test_deprecation
- "Test to" in docstring is unnecessary
- test_deprecation
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 9 years ago by
Author: | thijs → rodney757 |
---|---|
Status: | new → assigned |
Thanks for the review, I'll fix that docstring and merge.
comment:16 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
You can see an example deprecation in
twisted/conch/insults/__init__.py
and the applicable tests intwisted/conch/test/test_insults.py
.