Opened 5 years ago

Closed 5 years ago

#22808 closed enhancement (fixed)

absolute_import in Cython files

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

Status badges

Description


Change History (20)

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

one step in #22975

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

next step in #23068

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

next step in #23589

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

next step in #23641

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

next step in #23662

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

maybe the last step in #23745

but how can we check ?

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

Jeroen, do you have an idea how to find places where adding "absolute_import" is still needed ?

comment:8 Changed 5 years ago by Jeroen Demeyer

Do you mean for Python and Cython code in the Sage library or only Cython code?

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

Well, only Cython, to deal with this ticket. But of course one should also check for py files.

comment:10 Changed 5 years ago by Jeroen Demeyer

OK, both Python and Cython. I think it is best to treat Python and Cython the same here. Imports work the same way. The only difference is that Cython also has cimport.

I think that I only created this ticket because at some point you were adding from __future__ import absolute_import to a lot of Python files but not to Cython files. It seemed like you overlooked Cython files.

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

So what should we do here ?

I would like to have some certitude before closing, but I do not know how to test that everything is in order.

comment:12 Changed 5 years ago by Jeroen Demeyer

Actually, it might be easy to force Cython to use absolute_import everywhere. Maybe we can try the same thing with print_function.

comment:13 in reply to:  12 ; Changed 5 years ago by Frédéric Chapoton

Replying to jdemeyer:

Actually, it might be easy to force Cython to use absolute_import everywhere. Maybe we can try the same thing with print_function.

any idea on how to do that ?

comment:14 in reply to:  13 Changed 5 years ago by Jeroen Demeyer

Replying to chapoton:

any idea on how to do that ?

Yes.

comment:15 Changed 5 years ago by Jeroen Demeyer

Branch: u/jdemeyer/absolute_import_in_cython_files

comment:16 Changed 5 years ago by Jeroen Demeyer

Authors: Jeroen Demeyer
Commit: ac72aa133f95829dd5eeb3e239ef779261d20e27
Status: newneeds_review

Just one change required...


New commits:

ac72aa1Fix relative cimport

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

Great ! Does this mean that you tried to compile while forcing cython to use absolute import, and this was the only failure ?

comment:18 in reply to:  17 Changed 5 years ago by Jeroen Demeyer

Replying to chapoton:

Great ! Does this mean that you tried to compile while forcing cython to use absolute import, and this was the only failure ?

Yes. I didn't run doctests, but at least built and ran Sage.

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

Milestone: sage-8.0sage-8.1
Reviewers: Frédéric Chapoton
Status: needs_reviewpositive_review

Then let us consider that this ticket is fixed. Thanks !

comment:20 Changed 5 years ago by Volker Braun

Branch: u/jdemeyer/absolute_import_in_cython_filesac72aa133f95829dd5eeb3e239ef779261d20e27
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.