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

src/tox.ini: Check that there are no .all imports from namespace packages #32879

Closed
mkoeppe opened this issue Nov 15, 2021 · 18 comments
Closed

Comments

@mkoeppe
Copy link
Member

mkoeppe commented Nov 15, 2021

We add the following test to the relint linting workflow:
Library code should not import from sage.PAC.KAGE.all when sage.PAC.KAGE is a namespace package (see #32501)

CC: @kwankyu @fchapoton

Component: refactoring

Author: Matthias Koeppe

Branch: 928561e

Reviewer: Kwankyu Lee

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

@mkoeppe mkoeppe added this to the sage-9.5 milestone Nov 15, 2021
@mkoeppe mkoeppe changed the title src/tox.yml: Check that there are no .all imports from namespace packages src/tox.ini: Check that there are no .all imports from namespace packages Nov 15, 2021
@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 1, 2021

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 1, 2021

New commits:

7c7e3d2src/.relint.yml: Check that there are no .all imports from namespace packages

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 1, 2021

Commit: 7c7e3d2

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 1, 2021

Author: Matthias Koeppe

@kwankyu
Copy link
Collaborator

kwankyu commented Dec 2, 2021

comment:4

It works well. I am positive with the branch.

One small comment: I am dimly aware that there was a discussion somewhere(perhaps on sage-devel) on using two space characters (vs. one space character) to separate sentences. I don't remember the conclusion, but I feel uncomfortable with it, just like a typo. Hence please fix:

 - name: 'namespace_pkg_all_import: import from .all of a namespace package'
   hint: |
     Sage library code should not import from sage.PAC.KAGE.all when sage.PAC.KAGE is an implicit
-    Hint: namespace package.  Type import_statements("SOME_IDENTIFIER") to find a more specific import.
+    Hint: namespace package. Type import_statements("SOME_IDENTIFIER") to find a more specific import.
   pattern: 'from\s+sage[.](categories|misc|rings|combinat|graphs|interfaces|libs)[.]all\s+import'
   filePattern: '.*[.](py|pyx)$'

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 4, 2021

Changed commit from 7c7e3d2 to 928561e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 4, 2021

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

928561esrc/.relint.yml: Use 1 space after sentence end

@kwankyu
Copy link
Collaborator

kwankyu commented Dec 4, 2021

comment:6

Thank you.

@kwankyu
Copy link
Collaborator

kwankyu commented Dec 4, 2021

Reviewer: Kwankyu Lee

@vbraun
Copy link
Member

vbraun commented Dec 19, 2021

@fchapoton
Copy link
Contributor

Changed commit from 928561e to none

@fchapoton
Copy link
Contributor

comment:8

Imho, it is a very bad idea to introduce a check before it has been fully fixed. This just makes the linter useless.

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 29, 2021

comment:9

Useless, I don't think so. Sorry for ruining the green linter checkmarks on the tickets though

@tobiasdiez
Copy link
Contributor

comment:10

I have to agree. Now the linter workflow is falling for each ticket and it's no longer clear if that's because the ticket introduced some new issues or because of the all imports.

I propose to either remove the all imports soon or demote this check to a warning instead of an error (if that's possible with relint).

@mkoeppe
Copy link
Member Author

mkoeppe commented Jan 14, 2022

comment:11

Replying to @tobiasdiez:

I propose to either remove the all imports soon

+1, help welcome

@tobiasdiez
Copy link
Contributor

comment:12

Sorry, but if you don't plan to work on this yourself I would say we should demote it for now to a warning and fully activate it later.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jan 14, 2022

comment:13

Replying to @tobiasdiez:

... but if you don't plan to work on this yourself ...

Work is already underway on this, see tickets #32989, #32999, #33007, #33146, #33199, #33202

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

5 participants