py3: minor care for range

sagemath / sage

Main repository of SageMath

https://www.sagemath.org

Other

1.48k stars 488 forks source link

py3: minor care for range #24249

Closed fchapoton closed 6 years ago

fchapoton commented 7 years ago

part of #16081

Component: python3

Author: Frédéric Chapoton

Branch/Commit: 4642af3

Reviewer: Salvatore Stella

Issue created by migration from https://trac.sagemath.org/ticket/24249

fchapoton commented 7 years ago

Commit: a1f6942

fchapoton commented 7 years ago

Branch: u/chapoton/24249

fchapoton commented 7 years ago

New commits:

a1f6942 py3: some minor care for range

Etn40ff commented 7 years ago

comment:2

LGTM

Etn40ff commented 7 years ago

Reviewer: Salvatore Stella

jdemeyer commented 7 years ago

comment:3

Sorry, but I have to disagree here.

As I have mentioned several times before, you should really avoid "fixing" doctests. For example, matrix.diagonal(range(n)) really should work. If it doesn't work, then fix matrix.diagonal() to accept iterables instead of "fixing" the doctest. Fixing the doctests will actually make it harder to port to Python 3 because it just hides issues.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

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

5120785 trac 24249 diagonal matrix of range

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from a1f6942 to 5120785

jdemeyer commented 7 years ago

comment:5

Can we please have a separate ticket for the issues of diagonal_matrix(range(n)) and matrix([range(n)])? Stuffing this in a Python 3 ticket isn't practical for the discussion.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from 5120785 to 387df43

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

387df43 py3: some minor care for range

fchapoton commented 7 years ago

comment:7

I undid the change about diagonal matrices.

jdemeyer commented 7 years ago

comment:8

Same comment for giac.py: fix the code instead of the doctest.

fchapoton commented 7 years ago

comment:9

Really ? Do we really want to accept lists of range ?

jdemeyer commented 7 years ago

comment:10

Replying to @fchapoton:

Really ? Do we really want to accept lists of range ?

Of course, why not? It's the obvious generalization of a list of lists.

embray commented 7 years ago

comment:11

The natural abstraction from lists would probably be to support any sized Collection.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from 387df43 to 4642af3

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

4642af3 correct one range, plus pep8 details

fchapoton commented 7 years ago

comment:13

ok, I undid also the other change in giac.

now, this contains only one correction to range "in the code", plus a pep8 cleanup of the onlyu modified file.

Etn40ff commented 7 years ago

comment:14

Let's try again

vbraun commented 6 years ago

Changed branch from u/chapoton/24249 to 4642af3