Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#23147 closed enhancement (fixed)

py3: little cleanup of six and unicode

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

Status badges

Description

various small changes, split off from #23044

Change History (12)

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

Branch: u/chapoton/23147
Commit: d3f012d5001ffd879e08cfb5a382f6587d672f1e

New commits:

d3f012dsome cleanup of unicode and six

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

Cc: Jeroen Demeyer Travis Scrimshaw John Palmieri added
Status: newneeds_review

comment:3 Changed 5 years ago by John Palmieri

Reviewers: John Palmieri

Make this change and I'll be happy with it:

  • src/sage/algebras/commutative_dga.py

    diff --git a/src/sage/algebras/commutative_dga.py b/src/sage/algebras/commutative_dga.py
    index e571a51c30..a5071198f1 100644
    a b AUTHORS: 
    7171# (at your option) any later version.
    7272#                  http://www.gnu.org/licenses/
    7373#*****************************************************************************
    74 from __future__ import print_function, absolute_import
     74from __future__ import print_function
    7575from six import string_types
    7676
    7777from sage.misc.six import with_metaclass

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

Thanks for the review.

Concerning your suggestion, sorry, but why ?

Ideally, we should have this "absolute_import" in all of our .py files, to help for the transition for python3 and prevent possible regressions. Adding this should do no harm at all, once the file has been checked.

comment:5 Changed 5 years ago by John Palmieri

I didn't see the point to it. It is not necessary for from six import ... since we're not in the sage.misc directory. So why add extra lines that are not needed? But I don't care that much.

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

You are right. So maybe we should rather move on to work on something else, no ?

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

So ? positive review ?

comment:8 Changed 5 years ago by John Palmieri

Status: needs_reviewpositive_review

comment:9 Changed 5 years ago by Volker Braun

Branch: u/chapoton/23147d3f012d5001ffd879e08cfb5a382f6587d672f1e
Resolution: fixed
Status: positive_reviewclosed

comment:10 Changed 5 years ago by Erik Bray

Commit: d3f012d5001ffd879e08cfb5a382f6587d672f1e

This part doesn't make a lot of sense to me:

@@ -1635,7 +1636,7 @@ def _sage_getdoc_unformatted(obj):
     # not a 'getset_descriptor' or similar.
     if not isinstance(r, string_types):
         return ''
-    elif isinstance(r, unicode):
+    elif isinstance(r, text_type):  # unicode (py2) = str (py3)
         return r.encode('utf-8', 'ignore')
     else:
         return r

not your patch specifically--replacing unicode with text_type makes sense. But the r.encode that was there in the first place isn't right. It actually causes a crash for me at startup, because the return value from this function is later consumed by code that's expected text, not bytes. In other words, this code might have made slight sense from a Python 2 perspective, but for Python 3 it only makes sense here to leave str as str.

Last edited 5 years ago by Erik Bray (previous) (diff)

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

indeed, one should do the contrary (add decode in the other part, but this causes doctests failures..)

comment:12 in reply to:  11 Changed 5 years ago by Erik Bray

Replying to chapoton:

indeed, one should do the contrary (add decode in the other part, but this causes doctests failures..)

Right. I see now that you did this in your branch.

Note: See TracTickets for help on using tickets.