Opened 4 years ago

Closed 4 years ago

#22948 closed defect (fixed)

avoid "absolute_import" in tab completion

Reported by: vdelecroix Owned by:
Priority: major Milestone: sage-8.0
Component: misc Keywords:
Cc: dcoudert Merged in:
Authors: Vincent Delecroix Reviewers: David Coudert
Report Upstream: N/A Work issues:
Branch: 26617dd (Commits, GitHub, GitLab) Commit: 26617dd78d7230f73872841046713e3d9eed057d
Dependencies: Stopgaps:

Status badges

Description

As reported in this thread on sage-devel

sage: designs.<TAB>
absolute_import ...

Change History (12)

comment:1 Changed 4 years ago by vdelecroix

  • Branch set to u/vdelecroix/22948
  • Commit set to 1ae719287f3f44c3f18aac7bc2393afa45a7ffcb
  • Status changed from new to needs_review

New commits:

1ae719222948: del absolute_import at the end of catalogs

comment:2 Changed 4 years ago by tscrim

Big -1 on the (current) doctests as it is one more thing to update each time we add something to the respective catalog. I'm essentially in favor of removing them altogether. If you really want a doctest, then you're better checking if, e.g., 'absolute_import' in dir(crystals). I feel such a doctest, which is not there for all catalogs, is just going to create extra headaches.

comment:3 Changed 4 years ago by dcoudert

I also agree that absolute_import in dir(designs) would be safer.

I tried the current version and it solves the issue for designs.<TAB>and passes all tests on src/sage/coding, src/sage/combinat/crystals/, src/sage/combinat/designs/, src/sage/game_theory/, and src/sage/groups/.

comment:4 Changed 4 years ago by vdelecroix

All right. Will change it.

comment:5 Changed 4 years ago by git

  • Commit changed from 1ae719287f3f44c3f18aac7bc2393afa45a7ffcb to 26617dd78d7230f73872841046713e3d9eed057d

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

26617dd22948: del absolute_import at the end of catalogs

comment:6 Changed 4 years ago by vdelecroix

done

comment:7 Changed 4 years ago by dcoudert

  • Reviewers set to David Coudert
  • Status changed from needs_review to positive_review

For me this is good to go (passes all tests). Thank you.

comment:8 follow-up: Changed 4 years ago by embray

Rather than putting del everywhere (and likely forgetting in other modules) wouldn't it make more sense to update the __dir__ implementation to exclude it (and anything else from __futures__)?

Last edited 4 years ago by embray (previous) (diff)

comment:9 Changed 4 years ago by embray

Ah, of course, but this is a module. That said, surely there's somewhere to control what gets included in tab-completion (it doesn't include other globals like standard module imports does it?)

comment:10 in reply to: ↑ 8 Changed 4 years ago by vdelecroix

Replying to embray:

Rather than putting del everywhere (and likely forgetting in other modules) wouldn't it make more sense to update the __dir__ implementation to exclude it (and anything else from __futures__)?

It would! But I have no idea how to implement it.

comment:11 Changed 4 years ago by embray

Maybe it's not so simple after all. It seems there's nothing like that currently in Sage, and scant documentation on how to customize tab-completion in IPython (though it does appear to be possible). Maybe something worth working on (another possibility is that if we provide our own SageModule? class, as I've been meaning to do anyways, we can customize __dir__ for modules in Sage).

In the meantime, this is just limited to some "catalog" modules where it matters most so that's fine.

comment:12 Changed 4 years ago by vbraun

  • Branch changed from u/vdelecroix/22948 to 26617dd78d7230f73872841046713e3d9eed057d
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.