Opened 15 months ago

Closed 14 months ago

Last modified 13 months ago

#32879 closed enhancement (fixed)

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

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.5
Component: refactoring Keywords:
Cc: klee, chapoton Merged in:
Authors: Matthias Koeppe Reviewers: Kwankyu Lee
Report Upstream: N/A Work issues:
Branch: 928561e (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by mkoeppe)

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)

Change History (13)

comment:1 Changed 15 months ago by mkoeppe

Summary: src/tox.yml: Check that there are no .all imports from namespace packagessrc/tox.ini: Check that there are no .all imports from namespace packages

comment:2 Changed 14 months ago by mkoeppe

Branch: u/mkoeppe/src_tox_ini__check_that_there_are_no__all_imports_from_namespace_packages

comment:3 Changed 14 months ago by mkoeppe

Authors: Matthias Koeppe
Cc: klee chapoton added
Commit: 7c7e3d2342a9924a15abd3256967220fff7d52f9
Description: modified (diff)
Status: newneeds_review

New commits:

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

comment:4 Changed 14 months ago by klee

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)$'

comment:5 Changed 14 months ago by git

Commit: 7c7e3d2342a9924a15abd3256967220fff7d52f9928561ee6f2d2ebd543676b6d92f3f89d1a5f734

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

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

comment:6 Changed 14 months ago by klee

Reviewers: Kwankyu Lee
Status: needs_reviewpositive_review

Thank you.

comment:7 Changed 14 months ago by vbraun

Branch: u/mkoeppe/src_tox_ini__check_that_there_are_no__all_imports_from_namespace_packages928561ee6f2d2ebd543676b6d92f3f89d1a5f734
Resolution: fixed
Status: positive_reviewclosed

comment:8 Changed 13 months ago by chapoton

Commit: 928561ee6f2d2ebd543676b6d92f3f89d1a5f734

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

comment:9 Changed 13 months ago by mkoeppe

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

comment:10 Changed 13 months ago by gh-tobiasdiez

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).

comment:11 in reply to:  10 Changed 13 months ago by mkoeppe

Replying to gh-tobiasdiez:

I propose to either remove the all imports soon

+1, help welcome

comment:12 Changed 13 months ago by gh-tobiasdiez

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.

comment:13 in reply to:  12 Changed 13 months ago by mkoeppe

Replying to gh-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

Last edited 13 months ago by mkoeppe (previous) (diff)
Note: See TracTickets for help on using tickets.