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

Remove imports from sage.all and sage.rings.all in sage.rings #34189

Closed
mkoeppe opened this issue Jul 16, 2022 · 31 comments
Closed

Remove imports from sage.all and sage.rings.all in sage.rings #34189

mkoeppe opened this issue Jul 16, 2022 · 31 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 16, 2022

as found by ./sage -tox -e relint

... except for src/sage/rings/tests.py

Component: refactoring

Author: Matthias Koeppe

Branch/Commit: eecd577

Reviewer: Kwankyu Lee, Guillermo Moreno-Socías

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

@mkoeppe mkoeppe added this to the sage-9.7 milestone Jul 16, 2022
@mkoeppe mkoeppe changed the title Remove imports from sage.rings.all Remove imports from sage.rings.all in sage.rings Jul 16, 2022
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 16, 2022

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 16, 2022

Author: Matthias Koeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 16, 2022

New commits:

8431c6csrc/sage/rings: Remove imports from sage.rings.all

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 16, 2022

Commit: 8431c6c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 16, 2022

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

6df82e3src/sage/rings/rational_field.py: Remove imports from sage.rings.all
3e420f7src/sage/rings: Remove remaining imports from sage.rings.all

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 16, 2022

Changed commit from 8431c6c to 3e420f7

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 18, 2022

Changed commit from 3e420f7 to 635976a

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 18, 2022

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

7c732d3src/sage/rings/number_field/order.py: Incidental pyflakes fix
517980dsrc/sage/rings/rational_field.py: Remove two more .all imports
635976asrc/sage/rings/rational_field.py: Remove last .all import

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 18, 2022

Changed commit from 635976a to b9893f2

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 18, 2022

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

b9893f2sage.rings: Remove imports from sage.all

@mkoeppe mkoeppe changed the title Remove imports from sage.rings.all in sage.rings Remove imports from sage.all and sage.rings.all in sage.rings Jul 18, 2022
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 18, 2022

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

309726fsage.rings: Remove imports from sage.all

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 18, 2022

Changed commit from b9893f2 to 309726f

@GMS103
Copy link
Member

GMS103 commented Jul 25, 2022

comment:10

In src/sage/rings/quotient_ring.py, should

from sage.all import Infinity

be replaced by

from sage.rings.infinity import Infinity?

(If this is the case, I can look for other instances.)

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 25, 2022

comment:11

Yes, thanks for catching this. Not sure how it slipped through.

@GMS103
Copy link
Member

GMS103 commented Jul 25, 2022

comment:12

There is also
from sage.rings.all import IntegerModRing
in
src/sage/rings/padics/padic_template_element.pxi

but perhaps this is normal?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 25, 2022

Changed commit from 309726f to 96af429

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 25, 2022

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

96af429src/.relint.yml (namespace_pkg_all_import): Check all namespace packages from #33011, also check .pxi files

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 25, 2022

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

5047328src/sage/rings/quotient_ring.py: Remove .all import
eecd577src/sage/rings/padics/padic_template_element.pxi: Remove .all import

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 25, 2022

Changed commit from 96af429 to eecd577

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 25, 2022

comment:16

OK, fixed the tooling and fixed these two.

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 28, 2022

comment:17

In

pattern: 'from\s+sage(|[.](arith|categories|combinat|ext|graphs(|[.]decompositions)|interfaces|libs|matrix|misc|numerical(|[.]backends)|rings|sets))[.]all\s+import'

is the final . before all really optional?

The namespace package hierarchy is hardcoded into this pattern. Thus the pattern would need constant updating as the hierarchy evolves. Perhaps we need a central source of the namespace package hierarchy and this pattern could be constructed from the source. No?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 28, 2022

comment:18

Replying to @kwankyu:

In

pattern: 'from\s+sage(|[.](arith|categories|combinat|ext|graphs(|[.]decompositions)|interfaces|libs|matrix|misc|numerical(|[.]backends)|rings|sets))[.]all\s+import'

is the final . before all really optional?

These patterns are regular expressions. Square brackets are character sets, not optional items. [.] accepts the literal character . only

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 28, 2022

comment:19

Replying to @kwankyu:

Thus the pattern would need constant updating as the hierarchy evolves. Perhaps we need a central source of the namespace package hierarchy and this pattern could be constructed from the source.

In #34201, I mention a more semantic code analysis tool that could perhaps be used instead of this simple regular expression based check. But for now I'd like to keep this as simple as possible

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 28, 2022

comment:20

Replying to @mkoeppe:

Replying to @kwankyu:

In

pattern: 'from\s+sage(|[.](arith|categories|combinat|ext|graphs(|[.]decompositions)|interfaces|libs|matrix|misc|numerical(|[.]backends)|rings|sets))[.]all\s+import'

is the final . before all really optional?

These patterns are regular expressions. Square brackets are character sets, not optional items. [.] accepts the literal character . only

Right. I misunderstood.

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 28, 2022

comment:21

Replying to @mkoeppe:

In #34201, I mention a more semantic code analysis tool that could perhaps be used instead of this simple regular expression based check. But for now I'd like to keep this as simple as possible

Okay.

Then I am positive with this ticket.

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 28, 2022

Reviewer: Kwankyu Lee, ...

@GMS103
Copy link
Member

GMS103 commented Jul 28, 2022

comment:22

Then I am positive with this ticket.

Me too.

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 28, 2022

Changed reviewer from Kwankyu Lee, ... to Kwankyu Lee, Guillermo Moreno-Socías

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 28, 2022

comment:24

Thanks both!

@vbraun
Copy link
Member

vbraun commented Aug 1, 2022

Changed branch from u/mkoeppe/remove_imports_from_sage_rings_all to eecd577

@vbraun vbraun closed this as completed in 7ec4ff6 Aug 1, 2022
mkoeppe added a commit to mkoeppe/sage that referenced this issue Feb 17, 2023
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

4 participants