Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

py3: work on unicode in lazy_import #22946

Closed
fchapoton opened this issue May 4, 2017 · 28 comments
Closed

py3: work on unicode in lazy_import #22946

fchapoton opened this issue May 4, 2017 · 28 comments

Comments

@fchapoton
Copy link
Contributor

solving the first problem met in #22945

CC: @jdemeyer @tscrim

Component: python3

Keywords: unicode

Reviewer: Frédéric Chapoton

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

@fchapoton fchapoton added this to the sage-8.0 milestone May 4, 2017
@fchapoton
Copy link
Contributor Author

Commit: 48b2364

@fchapoton
Copy link
Contributor Author

New commits:

48b2364py3: work on unicode in lazy_import

@fchapoton
Copy link
Contributor Author

Branch: u/chapoton/22946

@fchapoton
Copy link
Contributor Author

Changed keywords from none to unicode

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 4, 2017

Changed commit from 48b2364 to b171480

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 4, 2017

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

b171480py3: unicode_literal in lazy_import

@fchapoton

This comment has been minimized.

@fchapoton
Copy link
Contributor Author

comment:5

bot is essentially green

@tscrim
Copy link
Collaborator

tscrim commented May 4, 2017

comment:6

Could you explain how importing unicode_literal allows you to use unicode in Python3? I would think you should use unicode_literal.

@tscrim
Copy link
Collaborator

tscrim commented May 4, 2017

comment:7

Also, is the spacing in the slices PEP8?

@fchapoton
Copy link
Contributor Author

comment:8

hmm, well. I have doubts, I must say.

Question 1: This is a pyx file. I nevertheless wonder whether cython understands "unicode" and "str" correctly in py2/py3 contexts

Question 2: I am sure for the +, not so sure about the :

I am only starting to try and touch the unicode question.. IMHO, this will be a large scale mess, maybe only marginally easier than the "cmp" mountain.

@tscrim
Copy link
Collaborator

tscrim commented May 5, 2017

comment:9

Question 1 is something Jeroen might be able to answer, I cannot. For Question 2, I would almost say not putting the spaces around the + as it looks somewhat strange. I more strongly disagree with the space after the : in a slice.

Hopefully the unicode problem won't be so pervasive as we don't use unicode in too many places.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 10, 2017

Changed commit from b171480 to 0554525

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 10, 2017

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

015eebbMerge branch 'u/chapoton/22946' in 8.0.b5
0554525trac 22946 undo pep8 change

@fchapoton
Copy link
Contributor Author

comment:11

Jeroen, any comment ?

@fchapoton
Copy link
Contributor Author

comment:12

ping ?

@tscrim
Copy link
Collaborator

tscrim commented May 15, 2017

comment:13

Or could the import of unicode_literals be a change to unicode like print_function is to print?

@tscrim
Copy link
Collaborator

tscrim commented May 16, 2017

comment:14

Okay, so I've looked a little more into unicode_literals, and it is a global flag that makes all strings into unicode (i.e., is like doing u'foo'). I'm slightly worried about potential side effects (well, not on my system since I don't have unicode file paths), so is it strictly necessary to include it?

Then again, this is Cython code, so Python rules need not apply.

@fchapoton
Copy link
Contributor Author

comment:15

so, is there still some objection to the current state of the branch ? patchbot is green..

@tscrim
Copy link
Collaborator

tscrim commented May 21, 2017

comment:16

I am okay with the branch fundamentally, but I am still a little nervous. So this is a positive review, but I don't want to merge it in such a late beta (maybe the next will be an rc). Instead I would want this merged in 8.1.beta0.

@jdemeyer
Copy link

comment:17

To answer the questions above:

  1. from future import unicode_literals is like changing all strings in the sources from "foo" to u"foo".

  2. Cython does understand str, bytes, unicode, basestring in all Python versions.

Note that this conflicts with #22755.

@fchapoton fchapoton modified the milestones: sage-8.0, sage-8.1 Jun 19, 2017
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 23, 2017

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

8b16e02Merge branch 'u/chapoton/22946' in 8.0.b12

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 23, 2017

Changed commit from 0554525 to 8b16e02

@fchapoton
Copy link
Contributor Author

comment:20

after merging with 8.0.b12, the branch is essentially empty. So I propose to set this to duplicate.

@fchapoton fchapoton removed this from the sage-8.1 milestone Jun 23, 2017
@jdemeyer
Copy link

Changed author from Frédéric Chapoton to none

@jdemeyer
Copy link

Reviewer: Frédéric Chapoton

@fchapoton
Copy link
Contributor Author

Changed branch from u/chapoton/22946 to none

@fchapoton
Copy link
Contributor Author

Changed commit from 8b16e02 to none

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants