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

Clean up publicly exported API with __all__ #514

Open
alchzh opened this issue Nov 12, 2024 · 3 comments
Open

Clean up publicly exported API with __all__ #514

alchzh opened this issue Nov 12, 2024 · 3 comments

Comments

@alchzh
Copy link
Collaborator

alchzh commented Nov 12, 2024

Because we're not defining __all__, files export many names from locations where they shouldn't be accessible.

This is compounded by ofrak.core reexporting names from submodules using star imports, leading to situations where scripts expect that system libraries like os are imported by from ofrak.core import *.

I think the lowest maintenance burden solution is enforcing __all__ usage in files.

Which files would be affected?
(almost) every python file

Does the proposed maintenance include non-doc string functional changes to the Python code?
Yes

Are you interested in implementing it yourself?
Yes

@rbs-jacob
Copy link
Member

I don't oppose this, but I do want to point out that if we use __all__, we will need to keep it updated every time we add new importable OFRAK components, attributes, classes, etc.

How should we ensure that it is kept up-to-date? Should there be a checkbox in every PR? A test that confirms new stuff is importable via CI?

@whyitfor
Copy link
Contributor

If I understand the proposal, putting __all__ each module will force the author to think about the file's public API.

@rbs-jacob, I think the problem of updating things exists already anyways. For example, on #508 the LLM analyzers are not exposed because they are not imported in https://github.com/redballoonsecurity/ofrak/blob/master/ofrak_core/ofrak/core/__init__.py.

Conceptually, it feels ok to me that an author needs to explicitly add something to the public API.

@rbs-jacob
Copy link
Member

I agree that it's not a big deal if the author needs to explicitly expose things, but it should not be assumed that contributors will know to do that without prompting from a failing test or a note in the PR template.

Arguably we should already have included such prompting, and the change associated with this issue seems like a good place to do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants