Opened 5 years ago

Closed 5 years ago

#24220 closed enhancement (fixed)

py3: fixing one bad use of zip

Reported by: Frédéric Chapoton Owned by:
Priority: major Milestone: sage-8.1
Component: python3 Keywords:
Cc: Travis Scrimshaw, Jeroen Demeyer, Thierry Monteil Merged in:
Authors: Frédéric Chapoton Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: aa6114b (Commits, GitHub, GitLab) Commit: aa6114b33a171202cef45357ae15d3f1aedab164
Dependencies: Stopgaps:

Status badges

Description

part of #16082

Change History (11)

comment:1 Changed 5 years ago by Frédéric Chapoton

Branch: u/chapoton/24220
Cc: Travis Scrimshaw Jeroen Demeyer Thierry Monteil added
Commit: 058707e335d171eb8c9cd62ff0989c9b0a998349
Status: newneeds_review

New commits:

058707epy3: fixing a bad zip (again)

comment:2 Changed 5 years ago by Travis Scrimshaw

I am not sure this is a problem. I thought that things like zip were (repeatedly) iterable. So the initial iteration should be okay, and delete_edges also iterates over the edges. Or can we only iterate over it once?

comment:3 Changed 5 years ago by Frédéric Chapoton

This fixes a failure found in the last patchbot run on #16082. And indeed in python3

>>> A=zip([4,3],[5,6])
>>> list(A)
[(4, 5), (3, 6)]
>>> list(A)
[]
Last edited 5 years ago by Frédéric Chapoton (previous) (diff)

comment:4 Changed 5 years ago by Travis Scrimshaw

I see. I think a better solution in this case is to duplicate the zip (with a comment saying in Python3, we need a fresh iterator). That way we keep the advantages in Python3, and we still have to make a copy of the list in Python2. What do you think?

comment:5 Changed 5 years ago by Frédéric Chapoton

Are we sure that "delete_edges" can take an iterator ?

comment:6 Changed 5 years ago by Travis Scrimshaw

The code is

for e in edges:
    self.delete_edge(e)
Last edited 5 years ago by Travis Scrimshaw (previous) (diff)

comment:7 Changed 5 years ago by git

Commit: 058707e335d171eb8c9cd62ff0989c9b0a998349aa6114b33a171202cef45357ae15d3f1aedab164

Branch pushed to git repo; I updated commit sha1. New commits:

aa6114btrac 24220 copy the zip

comment:8 Changed 5 years ago by Frédéric Chapoton

done, thanks


New commits:

aa6114btrac 24220 copy the zip

New commits:

aa6114btrac 24220 copy the zip

comment:9 Changed 5 years ago by Frédéric Chapoton

green bot

comment:10 Changed 5 years ago by Travis Scrimshaw

Reviewers: Travis Scrimshaw
Status: needs_reviewpositive_review

LGTM. Thanks.

comment:11 Changed 5 years ago by Volker Braun

Branch: u/chapoton/24220aa6114b33a171202cef45357ae15d3f1aedab164
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.