Skip to content

Make sure that __dir__ returns new copies of __all__ #135

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Mar 19, 2025
+24 −1
Merged

Make sure that __dir__ returns new copies of __all__ #135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2af5c69
Make sure that __dir__ returns a new copy of __all__
lagru Dec 10, 2024
df4f89a
Make linter happy
lagru Dec 10, 2024
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lazy_loader/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ def __getattr__(name):
raise AttributeError(f"No {package_name} attribute {name}")

def __dir__():
return __all__
return list(__all__)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very minor nit: I prefer to use the copy method over a new constructor. As a reader, it tells me that __all__ is already a list and not being converted.

Suggested change
return list(__all__)
return __all__.copy()

On a more theoretical note (as these lists will generally be small enough to be negligible), using a method over a constructor permits the implementation to skip generic logic to handle multiple input types, so upstream optimizations in Python can potentially have higher impact.

Some timeit logs (py3.13) 
In [2]: _dir = dir() In [4]: %timeit _dir[:] 41.7 ns ± 0.858 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each) In [5]: %timeit list(_dir) 46.5 ns ± 0.816 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each) In [6]: %timeit _dir.copy() 38.3 ns ± 0.667 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each) In [9]: x = list(range(100000)) In [10]: %timeit list(x) 273 μs ± 12.1 μs per loop (mean ± std. dev. of 7 runs, 1,000 loops each) In [11]: %timeit x.copy() 243 μs ± 8.06 μs per loop (mean ± std. dev. of 7 runs, 1,000 loops each) In [12]: %timeit x[:] 231 μs ± 11.1 μs per loop (mean ± std. dev. of 7 runs, 1,000 loops each) In [13]: x = list(range(1000)) In [14]: %timeit list(x) 1.13 μs ± 15 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each) In [15]: %timeit x.copy() 1.13 μs ± 16 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each) In [16]: %timeit x[:] 1.15 μs ± 16.9 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each) In [17]: x = list(range(10)) In [18]: %timeit list(x) 36.3 ns ± 0.642 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each) In [19]: %timeit x.copy() 26.5 ns ± 0.504 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each) In [20]: %timeit x[:] 30.6 ns ± 0.748 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)
Copy link
Member Author

@lagru lagru Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was debating that myself but chose to use list() over .copy() to be consistent with

lazy-loader/lazy_loader/__init__.py

Line 99 in 4e78314

return __getattr__, __dir__, list(__all__)

I'm happy to update both to .copy() if desired. 😊
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that's fine with me. Wasn't going to suggest it since it's a little out-of-scope, but I don't see the harm in changing it in passing.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind updating, @lagru, then we can merge this.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the copy approach still support the case of __all__ being a tuple?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should, because the __all__ in question is a list generated inside this function.

if os.environ.get("EAGER_IMPORT", ""):
for attr in set(attr_to_modules.keys()) | submodules:
Expand Down
23 changes: 23 additions & 0 deletions lazy_loader/tests/test_lazy_loader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,29 @@ def test_lazy_attach():
assert locls[k] == v


def test_lazy_attach_returns_copies():
_get, _dir, _all = lazy.attach(
__name__, ["my_submodule", "another_submodule"], {"foo": ["some_attr"]}
)
assert _dir() is not _dir()
assert _dir() == _all
assert _dir() is not _all

expected = ["another_submodule", "my_submodule", "some_attr"]
assert _dir() == expected
assert _all == expected
assert _dir() is not _all

_dir().append("modify_returned_list")
assert _dir() == expected
assert _all == expected
assert _dir() is not _all

_all.append("modify_returned_all")
assert _dir() == expected
assert _all == [*expected, "modify_returned_all"]


def test_attach_same_module_and_attr_name():
from lazy_loader.tests import fake_pkg

Expand Down
Loading