Opened 2 years ago

Closed 2 years ago

#5785 task closed fixed (fixed)

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

Reported by: thijs Owned by: thijs
Priority: normal Milestone: Python 3.3 Minimal
Component: core Keywords:
Cc: thijs Branch: branches/versions-file-5785
(diff, github, buildbot, log)
Author: itamarst Launchpad Bug:

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 2 years ago.
versions-5785-2.patch (6.0 KB) - added by thijs 2 years ago.

Download all attachments as: .zip

Change History (14)

Changed 2 years ago by thijs

comment:1 Changed 2 years ago by thijs

  • Keywords review added

comment:2 follow-up: Changed 2 years ago by 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) :-)

Changed 2 years ago by thijs

comment:3 in reply to: ↑ 2 Changed 2 years ago by thijs

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 2 years ago by vperic

  • Keywords review removed
  • Owner set to thijs

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

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

(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 follow-up: Changed 2 years ago by vperic

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

  • Status changed from reopened to new

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 follow-up: Changed 2 years ago by vperic

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

  • Status changed from new to assigned

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 2 years ago by itamar

  • Milestone changed from Python-3.x to Python 3.3 Minimal

comment:11 Changed 2 years ago by itamarst

  • Author set to itamarst
  • Branch set to branches/versions-file-5785

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

comment:12 Changed 2 years ago by exarkun

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

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