redballoonsecurity / ofrak

OFRAK: unpack, modify, and repack binaries.
https://ofrak.com
Other
1.88k stars 128 forks source link

Clean up publicly exported API with `__all__` #514

Open alchzh opened 2 weeks ago

alchzh commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 https://github.com/redballoonsecurity/ofrak/pull/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 commented 2 weeks ago

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.