[loader] Properly walk module directories #990
Conversation
Tested fairly thoroughly, works with reloading, fresh starts. Should ignore compiled python (__pycache__) directories, as well as both hidden files and directories. Works with pypi installed third party modules, should work with third party packaged modules put directly in the modules directory. Standalone python files in the modules directory work as before.
|
+1 not sure why this could hurt, and it only allows for better organisation of custom modules I will test this on Saturday. |
|
@thomastanck Is it Saturday yet? 😸 (Translation: Are you still able to test this?) Note to self: Squash out 601936d before merging, assuming the code requires no other changes. |
|
Sorry about that... it's Sunday already :( |
|
Punting to next major update: There is currently no one with prior experience to review this, and the known issues (ignoring symlinks, failing on packaged modules) really should be solved before merging. |
There was a problem hiding this comment.
This is mostly a comment for when this gets seriously looked at in the future, but I might as well also officially mark this as needing changes (because of the reported issues in the main thread: not following symlinks and not loading packaged modules correctly).
| result = get_module_description(path) | ||
| if result: | ||
| modules[result[0]] = result[1:] | ||
| for root, dirs, files in os.walk(directory): |
There was a problem hiding this comment.
Following symlinks is probably as simple as adding followlinks=True to this function call.
| for root, dirs, files in os.walk(directory): | ||
| # Ignore hidden folders/files and __ prefixed (__pycache__) | ||
| files = [f for f in files if not f[0] == '.'] | ||
| dirs[:] = [d for d in dirs if not d[0] == '.'] |
There was a problem hiding this comment.
I know this syntax works, but here it is equivalent to this less confusing one:
dirs = [d for d in dirs if not d[0] == '.']
| # Ignore hidden folders/files and __ prefixed (__pycache__) | ||
| files = [f for f in files if not f[0] == '.'] | ||
| dirs[:] = [d for d in dirs if not d[0] == '.'] | ||
| dirs[:] = [d for d in dirs if not d.startswith('__')] |
There was a problem hiding this comment.
It is possible to merge both filter in a single iteration:
dirs = [
d for d in dirs
if not d.startswith('.') and not d.startswith('__')
]
| modules[result[0]] = result[1:] | ||
| else: | ||
| non_pkg_dirs.append(folder) | ||
| dirs[:] = non_pkg_dirs |
There was a problem hiding this comment.
I'm not sure we need to store non_pkg_dirs and why it replaces dirs either.
|
@Exirel Do you want to take this over in a new PR? You could just |
|
@dgw let's do this then! |
dgw
added
Tweak
Stale
|
Closing in favor of #1479 |
dgw
added
Replaced
Tested fairly thoroughly, works with reloading, fresh starts. Should ignore compiled python (
__pycache__) directories, as well as both hidden files and directories. Works with pypi installed third party modules,should work with third party packaged modules put directly in the modules directory. Standalone python files in the modules directory work as before.Edit: It will currently ignore symlinks, dunno if we should do that or not.
Edit2: It doesn't work if you drop a python package in the modules directory, not really sure what subtlety is messing that up.