#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: |
Description (last modified by )
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
Summary: | 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 |
---|
comment:2 Changed 14 months ago by
Branch: | → u/mkoeppe/src_tox_ini__check_that_there_are_no__all_imports_from_namespace_packages |
---|
comment:3 Changed 14 months ago by
Authors: | → Matthias Koeppe |
---|---|
Cc: | klee chapoton added |
Commit: | → 7c7e3d2342a9924a15abd3256967220fff7d52f9 |
Description: | modified (diff) |
Status: | new → needs_review |
comment:4 Changed 14 months ago by
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
Commit: | 7c7e3d2342a9924a15abd3256967220fff7d52f9 → 928561ee6f2d2ebd543676b6d92f3f89d1a5f734 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
928561e | src/.relint.yml: Use 1 space after sentence end
|
comment:6 Changed 14 months ago by
Reviewers: | → Kwankyu Lee |
---|---|
Status: | needs_review → positive_review |
Thank you.
comment:7 Changed 14 months ago by
Branch: | u/mkoeppe/src_tox_ini__check_that_there_are_no__all_imports_from_namespace_packages → 928561ee6f2d2ebd543676b6d92f3f89d1a5f734 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
comment:8 Changed 13 months ago by
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
Useless, I don't think so. Sorry for ruining the green linter checkmarks on the tickets though
comment:10 follow-up: 11 Changed 13 months ago by
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 Changed 13 months ago by
comment:12 follow-up: 13 Changed 13 months ago by
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.
New commits:
src/.relint.yml: Check that there are no .all imports from namespace packages