feat: check for __all__ presence in __init__.py files#766
feat: check for __all__ presence in __init__.py files#766AcquaDiGiorgio wants to merge 25 commits intoDIRACGrid:mainfrom
Conversation
I didn't see any standard way of defining |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
chrisburr
left a comment
chrisburr
left a comment
There was a problem hiding this comment.
Yet another fake review for me to be able to test something 😄
|
I don't know how much of an improvement this is, but we might not want to use importlib for this task. To import an spec = importlib.util.spec_from_file_location(module_name, file)
module = importlib.util.module_from_spec(spec)
spec.loader.exec_module(module)Here, While importing the module, if the However, as a counterpart of this method, importing the module each time is quite slow, and I don't see any issues on using regex. Also, I found a wild |
|
Note: I keep the PR as a draft to not interrupt whatever test you are doing, but is ready to review. |
No, I don't think so. Can you remove it please? |
|
This every dependency I have extracted from the imports on every file: Sometimes a module is imported, such as I tried to put in the from __future__ import annotations
__all__ = [
"SecurityProperty",
"UnevaluatedProperty",
"PROXY_MANAGEMENT",
"GENERIC_PILOT",
"JOB_ADMINISTRATOR",
"NORMAL_USER",
"JOB_SHARING",
"s3_bucket_exists",
"generate_presigned_upload",
"s3_bulk_delete_with_retry",
"s3_object_exists",
"find_compatible_platforms",
"ServiceSettingsBase",
"AuthSettings",
"DevelopmentSettings",
"SandboxStoreSettings",
"SqlalchemyDsn",
"DiracEntryPoint",
"select_from_extension",
"supports_extending",
"DiracError",
"DiracHttpResponseError",
"NotReadyError",
"InvalidCredentialsError",
"TokenNotFoundError",
"AuthorizationError",
"IAMClientError",
"IAMServerError",
"PendingAuthorizationError",
"SandboxAlreadyAssignedError",
"SandboxNotFoundError",
"SandboxAlreadyInsertedError",
"InvalidQueryError",
"ConfigSource",
"Config",
"ConfigSourceUrl",
"DiracxPreferences",
"get_diracx_preferences",
"OutputFormats",
"EXPIRES_GRACE_SECONDS",
"serialize_credentials",
"EXPIRES_GRACE_SECONDS",
"serialize_credentials",
"dotenv_files_from_environment",
"read_credentials",
"write_credentials",
]
from .config import Config, ConfigSource, ConfigSourceUrl
from .exceptions import (
AuthorizationError,
DiracError,
DiracHttpResponseError,
IAMClientError,
IAMServerError,
InvalidCredentialsError,
InvalidQueryError,
NotReadyError,
PendingAuthorizationError,
SandboxAlreadyAssignedError,
SandboxAlreadyInsertedError,
SandboxNotFoundError,
TokenNotFoundError,
)
from .extensions import (
DiracEntryPoint,
select_from_extension,
supports_extending,
)
from .preferences import (
DiracxPreferences,
OutputFormats,
get_diracx_preferences,
)
from .properties import (
GENERIC_PILOT,
JOB_ADMINISTRATOR,
JOB_SHARING,
NORMAL_USER,
PROXY_MANAGEMENT,
SecurityProperty,
UnevaluatedProperty,
)
from .resources import find_compatible_platforms
from .s3 import (
generate_presigned_upload,
s3_bucket_exists,
s3_bulk_delete_with_retry,
s3_object_exists,
)
from .settings import (
AuthSettings,
DevelopmentSettings,
SandboxStoreSettings,
ServiceSettingsBase,
SqlalchemyDsn,
)
from .utils import (
EXPIRES_GRACE_SECONDS,
dotenv_files_from_environment,
read_credentials,
serialize_credentials,
write_credentials,
)Is this what we want? |
|
Here is an example of what we want I think:
I didn't check all the sub directories and modules, it's just to give you a sense of what we should probably aim for. |
|
Yeah, that makes sense, the example of For example, the diracx-routers project: You said that in this case If so, those dependencies should be in the the |
Some of them could be if there are meant to be used in the extensions indeed. You can have a look at the If you have any doubt, you can present the different options with pros and cons 🙂 |
|
Okay, after looking through lhcbdiracx a bit , then maybe the best way of proceeding would be:
More or less the same way as you said, but taking into account the modules at root I could change the pre-commit hook to check modules at the root if we go with this |
Things to check out
|
|
This ended up being a huge change, we should discuss it further |
|
I am ok with your proposed convention (as long as it is correctly documented in
Then remove it please
This is likely expected is meant to be discovered and merged by the auto-generated client code. Should probably stay as is and excluded by the pre-commit.
Well, I think you could make it empty to follow your convention, and just change the imports. What do you think?
I think it's ok to empty it. |
AcquaDiGiorgio
changed the title
AcquaDiGiorgio
force-pushed
the
issue-426-__all__-in-__init__
branch
from
a4262f0 to
0fdbb25
Compare
fix: Exclude automatically generated __init__ files
AcquaDiGiorgio
force-pushed
the
issue-426-__all__-in-__init__
branch
from
0af7880 to
53b4db6
Compare
aldbr
left a comment
aldbr
left a comment
There was a problem hiding this comment.
Looks almost ok. Just a few additional general comments:
- we could potentially add these ruff rules (complementary to your work):
| run: | | ||
| pip install typer pyyaml gitpython packaging | ||
| git clone https://github.com/DIRACGrid/DIRAC.git -b "${{ matrix.dirac-branch }}" /tmp/DIRACRepo | ||
| git clone https://github.com/AcquaDiGiorgio/DIRAC.git -b "diracx-issue-426-__all__-in-__init__" /tmp/DIRACRepo |
There was a problem hiding this comment.
Can you revert the changes here please?
| - name: Start demo | ||
| run: | | ||
| git clone https://github.com/DIRACGrid/diracx-charts.git ../diracx-charts | ||
| git clone -b "diracx-issue-426-__all__-in-__init__" https://github.com/AcquaDiGiorgio/diracx-charts.git ../diracx-charts |
|
|
||
| from __future__ import annotations | ||
|
|
||
| __all__ = ["BaseAccessPolicy", "check_permissions", "open_access"] |
There was a problem hiding this comment.
This won't be checked by your script I think. Is that expected?
There was a problem hiding this comment.
Yeah, it only checks init files.
I will look into this, because it should also look for the dunder in the files at the root of each project, now that you mention it.
There was a problem hiding this comment.
I don't think it's necessary to add __all__ to the tests directories. What do you think?
There was a problem hiding this comment.
I don't think so either, I will add it to the exclude section
|
|
||
| """ | ||
|
|
||
| from __future__ import annotations |
There was a problem hiding this comment.
Since you now have added that line, this may be removed in pyproject.toml, to investigate:
[tool.ruff.lint.extend-per-file-ignores]
"diracx-routers/src/diracx/routers/access_policies.py" = ["I002"]
There was a problem hiding this comment.
Yeah, it can be removed, since the pull request has been merged already.
|
|
||
| ci: | ||
| skip: [generate-pixi-docs, settings-doc-check] | ||
| skip: [generate-pixi-docs, settings-doc-check, check-init-files] |
There was a problem hiding this comment.
I don't think we want to ignore that check in the CI.
What if you run with language: python and entry: python ... instead? You can mention that during Ddev to see what other devs think about that
|
See https://github.com/DIRACGrid/diracx/pull/912/changes to potentially run your script with |
invalid-all-object (PLE0604) is already in use. In the |
3 participants
See #426
I tried something more sofisticated in python using
importlibto check the variables in the module, both instantiated and imported, but there were multiple situations that were extremely difficult to check.We also might want to set a common way of defining this variable, using either tuples or lists:
__all__ = ["router"]__all__ = ("JobParametersDB",)