Opened 5 years ago

Closed 4 years ago

#5785 task closed fixed (fixed)

Replace usage of file with open in twisted.python.versions

Reported by: Thijs Triemstra Owned by: Thijs Triemstra
Priority: normal Milestone: Python 3.3 Minimal
Component: core Keywords:
Cc: Thijs Triemstra Branch: branches/versions-file-5785
branch-diff, diff-cov, branch-cov, buildbot
Author: itamarst

Description

twisted.python.versions contains 2 file calls that are deprecated in Python 3, and should be replaced with open instead.

twisted/python/versions.py:212:                format = file(formatFile).read().strip()
twisted/python/versions.py:223:            entries = file(entriesFile)

Attached patch replaces these calls and also brings code coverage from 92 to 98%.

Attachments (2)

versions-5785.patch (4.4 KB) - added by Thijs Triemstra 5 years ago.
versions-5785-2.patch (6.0 KB) - added by Thijs Triemstra 5 years ago.

Download all attachments as: .zip

Change History (14)

Changed 5 years ago by Thijs Triemstra

Attachment: versions-5785.patch added

comment:1 Changed 5 years ago by Thijs Triemstra

Keywords: review added

comment:2 Changed 5 years ago by Antoine Pitrou

I can't comment on the new tests, but the change in twisted.python.versions looks obviously fine. Of course, you could have taken the opportunity to use a "with" statement (and perhaps made explicit the "r" argument to open(), but that's a personal preference) :-)

Changed 5 years ago by Thijs Triemstra

Attachment: versions-5785-2.patch added

comment:3 in reply to:  2 Changed 5 years ago by Thijs Triemstra

Replying to antoine:

I can't comment on the new tests, but the change in twisted.python.versions looks obviously fine. Of course, you could have taken the opportunity to use a "with" statement (and perhaps made explicit the "r" argument to open(), but that's a personal preference) :-)

Thanks for the review, I added the 'r' argument in the latest patch.

comment:4 Changed 5 years ago by Vladimir Perić

Keywords: review removed
Owner: set to Thijs Triemstra

This looks good to me. There's also some overlap with ticket #5627, but lets wait for this to get in and I'll take care of the other ticket. I guess a topfile should be added and tests run, but other than that +1 from me.

comment:5 Changed 5 years ago by Thijs Triemstra

Resolution: fixed
Status: newclosed

(In [34948]) Apply versions-5785-2.patch: Replace the usage of file with open in twisted.python.versions for Python 3 compatibility.

Author: thijs Reviewer: antoine, vperic Fixes: #5785 Refs #5627

comment:6 Changed 5 years ago by Vladimir Perić

Resolution: fixed
Status: closedreopened

(In [34973]) Revert r34948: Apply versions-5785-2.patch

The commit added a test, test_dontAllowOtherObjects, which compares a Version with a string object. It is however not reliable, as it compares memory addresses, so revert the whole commit.

Reopens: #5785

comment:7 in reply to:  6 Changed 5 years ago by Thijs Triemstra

Status: reopenednew

Replying to vperic:

(In [34973]) Revert r34948: Apply versions-5785-2.patch

The commit added a test, test_dontAllowOtherObjects, which compares a Version with a string object. It is however not reliable, as it compares memory addresses, so revert the whole commit.

Reopens: #5785

Ugh. I will prepare a new patch for this.

comment:8 Changed 5 years ago by Vladimir Perić

Hi, in #5627 I submitted a patch to remove cmp which has a similar test, so please don't duplicate work.

comment:9 in reply to:  8 Changed 5 years ago by Thijs Triemstra

Status: newassigned

Replying to vperic:

Hi, in #5627 I submitted a patch to remove cmp which has a similar test, so please don't duplicate work.

I'm aware of this and will hold off with this work until #5627 has landed.

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

Milestone: Python-3.xPython 3.3 Minimal

comment:11 Changed 5 years ago by itamarst

Author: itamarst
Branch: branches/versions-file-5785

(In [35325]) Branching to 'versions-file-5785'

comment:12 Changed 4 years ago by Jean-Paul Calderone

Resolution: fixed
Status: assignedclosed

(In [35431]) Merge versions-python3-5917-3

Author: exarkun Reviewer: itamar Fixes: #5917 Fixes: #5785 Fixes: #5886

Port twisted.python.versions to Python 3. Also add a bit of missing test coverage and restore the definition of twisted.__version__ on Python 3.

Note: See TracTickets for help on using tickets.