Opened 5 years ago

Closed 5 years ago

#5688 task closed fixed (fixed)

Replace "x.has_key(y)" with "y in x" where covered by tests

Reported by: Vladimir Perić Owned by: Vladimir Perić
Priority: normal Milestone: Python-3.x
Component: core Keywords: py3k
Cc: Branch: branches/haskey-removal-5688
branch-diff, diff-cov, branch-cov, buildbot
Author: vperic

Description

The dict.has_key(k) syntax is deprecated (and completely gone by Python 3) in favor of "k in dict". Ticket #4053 attempted to fix this, but was stalled due to making many untested changes (though these are all trivial). As such, I've decided to take a different approach: this ticket only fixes the lines covered by the tests suite. I determined this by running the test suite with the "-3" option, and then checking each line individually.

Attachments (1)

haskey-tested.patch (20.7 KB) - added by Vladimir Perić 5 years ago.

Download all attachments as: .zip

Change History (8)

Changed 5 years ago by Vladimir Perić

Attachment: haskey-tested.patch added

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

Keywords: review removed
Owner: set to Vladimir Perić

Thanks for working on this! Sticking to tested stuff in one ticket and untested stuff in another ticket seems like a reasonable approach. A few more minor changes needed:

  1. Update coding standard document to note that has_key shouldn't be used.
  2. Add a .misc newsfile.
  3. Once ticket is merged, add a comment to #4053 indicating its scope is now limited to instances that are untested.

Next steps:

  1. Do the above changes in a Twisted branch using the combinator infrastructure, mkbranch and so on (I'll make sure you get commit access).
  2. Run force-builds.py to make sure it passes on all platforms (http://twistedmatrix.com/trac/wiki/CommitterCheckList has a link). There may BTW be e.g. Windows-only tests that need has_key fixed as well, but those can be done elsewhere in another ticket, don't have to do all at once.
  3. Merge.

comment:2 Changed 5 years ago by Vladimir Perić

Branch: branches/haskey-removal-5688

(In [34486]) Create branch haskey-removal-5688

comment:3 Changed 5 years ago by Vladimir Perić

Ok, so, assuming I did everything correctly, the branch is ok (with a topfile but without coding standard changes; I'll compile several important points and submit a new ticket) and I'm the builds should be at: Build Results

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

If you meant to submit this for review again you should add "review" keyword.

You're still missing the news file, I think? Once that is in merge.

comment:5 Changed 5 years ago by Vladimir Perić

Keywords: review added
Owner: Vladimir Perić deleted

Ok, added the topfile, run the tests again, but there are still failures in three of the bots. I don't really see code of mine could have caused any of this, but as this is my first time merging, I'd still prefer to have someone look over it.

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

Keywords: review removed
Owner: set to Vladimir Perić

Those look like the usual failures (not sure what's up with lucid buildslave, but that also looks unrelated to you). So looks ready for merging.

comment:7 Changed 5 years ago by Vladimir Perić

Resolution: fixed
Status: newclosed

(In [34521]) Merge haskey-removal-5688

Author: vperic Reviewer: itamar Fixes: #5688

The "dict.has_key(k)" syntax is deprecated in Python 3, use the new "k in dict" syntax instead. This change only covers the uses of has_key covered by the tests.

In one case, the has_key function itself was being passed; pass the internal contains function instead.

Note: See TracTickets for help on using tickets.