Opened 7 years ago

Closed 4 years ago

#16409 closed enhancement (fixed)

2to3 refactoring tool has warnings for some code

Reported by: wluebbe Owned by:
Priority: minor Milestone: sage-8.0
Component: python3 Keywords: python3
Cc: tscrim, jdemeyer, jhpalmieri Merged in:
Authors: Frédéric Chapoton Reviewers: André Apitzsch
Report Upstream: N/A Work issues:
Branch: ba2fdb1 (Commits, GitHub, GitLab) Commit: ba2fdb126734c6c7b603e2d051abc0e96b93bf22
Dependencies: Stopgaps:

Status badges

Description

While running the Python 2to3 on the Sage code base some warning are emitted:

  • fixer= absolute_import
    ### In file sage/misc/sage_timeit.py ###
    Line 199: absolute and local imports together
    
  • fixer= map
    ### In file sage/geometry/polyhedron/constructor.py ###
    Line 364: You should use a for loop here
    ### In file sage/geometry/polyhedron/ppl_lattice_polytope.py ###
    Line 1078: You should use a for loop here
    
  • fixer= next
    ### In file sage/combinat/integer_list.py ###
    Line 322: Calls to builtin next() possibly shadowed by global binding
    
  • fixer= operator
    ### In file sage/combinat/sf/sfa.py ###
    Line 3520: You should use 'operator.mul(lcm(pair), times=gcd(pair))' here.
    ### In file sage/combinat/species/generating_series.py ###
    Line 724: You should use 'operator.mul(lcm(pair), times=gcd(pair))' here.
    

Change History (12)

comment:1 Changed 7 years ago by wluebbe

  • Branch set to u/wluebbe/ticket/16409
  • Cc tscrim added
  • Commit set to 3bd3bc57abb84722581833452ecc8943f2641fb5
  • Status changed from new to needs_info
  • I fixed "absolute_import" and "map" cases. It tests OK.
  • I will check the "operator" case later.

The redefinition of next() in sage/combinat/integer_list.py has the text INTERNAL FUNCTION! DO NOT USE DIRECTLY!. At the top of the module it says

IMPORTANT NOTE (2009/02):
The internal functions in this file will be deprecated soon.
Please only use them through :class:`IntegerListsLex`.

Travis, what would you suggest? - The deprecation has not yet happened, but I do not find it obvious how to fix ...


New commits:

879f41btrac #16409: 2to3 refactoring tool warnings - fix "absolute_import"
3bd3bc5trac #16409: 2to3 refactoring tool warnings - fix "map"

comment:2 Changed 7 years ago by tscrim

Short-term solution would be to rename next to next_object or something like this (it's also at least used in combinat/integer_vector.py). The true deprecation solution is going to be much harder (I can try to do this if you want, but it would take a lot longer to do).

comment:3 Changed 7 years ago by wluebbe

But isn't {{{next()}} an element if the public API? So renaming would be no option.

I'm currently thinking it would be best to ignore this warning now.

comment:4 Changed 7 years ago by tscrim

It's not in the public (Sage) namespace, so we can rename it without having to deprecate it. However if you think it's fine to just ignore it, then that's fine (and I'll do the refactoring later this year).

comment:5 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:6 Changed 5 years ago by chapoton

  • Branch changed from u/wluebbe/ticket/16409 to public/16409
  • Commit changed from 3bd3bc57abb84722581833452ecc8943f2641fb5 to 9080506a2f8fa309b175070507953514600ac543

New commits:

9080506Merge branch 'u/wluebbe/ticket/16409' into 7.3.b6

comment:7 Changed 5 years ago by chapoton

  • Component changed from build to python3

comment:8 Changed 4 years ago by chapoton

  • Authors set to Frédéric Chapoton
  • Branch changed from public/16409 to u/chapoton/16409
  • Commit changed from 9080506a2f8fa309b175070507953514600ac543 to ba2fdb126734c6c7b603e2d051abc0e96b93bf22
  • Milestone changed from sage-6.4 to sage-8.0
  • Status changed from needs_info to needs_review

only the "operator" fixer issue still makes (some) sense.


New commits:

ba2fdb1py3: silent some 2to3 warnings

comment:9 Changed 4 years ago by chapoton

  • Cc jdemeyer jhpalmieri added

bot is essentially green. Should be an easy review, I think

comment:10 Changed 4 years ago by chapoton

ping ?

comment:11 Changed 4 years ago by aapitzsch

  • Reviewers set to André Apitzsch
  • Status changed from needs_review to positive_review

LGTM.

comment:12 Changed 4 years ago by vbraun

  • Branch changed from u/chapoton/16409 to ba2fdb126734c6c7b603e2d051abc0e96b93bf22
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.