Opened 3 years ago

Closed 2 years ago

#22808 closed enhancement (fixed)

absolute_import in Cython files

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

Description


Change History (20)

comment:1 Changed 3 years ago by chapoton

one step in #22975

comment:2 Changed 3 years ago by chapoton

next step in #23068

comment:3 Changed 2 years ago by chapoton

next step in #23589

comment:4 Changed 2 years ago by chapoton

next step in #23641

comment:5 Changed 2 years ago by chapoton

next step in #23662

comment:6 Changed 2 years ago by chapoton

maybe the last step in #23745

but how can we check ?

comment:7 Changed 2 years ago by chapoton

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

comment:8 Changed 2 years ago by jdemeyer

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

comment:9 Changed 2 years ago by chapoton

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

comment:10 Changed 2 years ago by jdemeyer

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 2 years ago by 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 follow-up: Changed 2 years ago by jdemeyer

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 ; follow-up: Changed 2 years ago by 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 2 years ago by jdemeyer

Replying to chapoton:

any idea on how to do that ?

Yes.

comment:15 Changed 2 years ago by jdemeyer

  • Branch set to u/jdemeyer/absolute_import_in_cython_files

comment:16 Changed 2 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Commit set to ac72aa133f95829dd5eeb3e239ef779261d20e27
  • Status changed from new to needs_review

Just one change required...


New commits:

ac72aa1Fix relative cimport

comment:17 follow-up: Changed 2 years ago by 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 2 years ago by jdemeyer

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 2 years ago by chapoton

  • Milestone changed from sage-8.0 to sage-8.1
  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

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

comment:20 Changed 2 years ago by vbraun

  • Branch changed from u/jdemeyer/absolute_import_in_cython_files to ac72aa133f95829dd5eeb3e239ef779261d20e27
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.