Switch Python import machinery to focus primarily on Python modules #1176
Conversation
chrisrink10
force-pushed
the
feature/use-module
branch
from
2a4504a to
5d91420
Compare
There was a problem hiding this comment.
Tests looks good.
Out of interest, would it be better to clean up the namespace after the test has run?
| assert result.ret != 0 | ||
| result.stdout.fnmatch_lines( | ||
| [ | ||
| "*basilisp.lang.compiler.exception.CompilerException: unable to resolve symbol 'require'*" |
There was a problem hiding this comment.
Tweaked this check which now matches both how things work both in Basilisp at the REPL as well as in Clojure (I believe).
| if not paths: | ||
| top = None | ||
| for p in file.parents: | ||
| if _is_package(p): | ||
| top = p | ||
| else: | ||
| break | ||
|
|
||
| if top is None or top == file.parent: | ||
| return [file.stem] |
There was a problem hiding this comment.
I'm not convinced this branch is still necessary.
|
I'm hoping this can fix the issues documented in #1155 and #1165 in an alternative way to PR #1172. The idea being that we just let the namespace control its own name rather than trying to the name out from a filesystem location. @ikappaki take a look when you have a minute and let me know what you think. |
Hi @chrisrink10, I'm not able to follow up on the particulars in detail of what the updated code does, but your description makes sense. My only concern is whether a Could you also consider updating the docs with the section I provided in the other patch? Feel free to make any extensive adjustments you find necessary. I'll test this PR with various project configurations and report my findings here later today. Thanks |
Tested with
Thus, everything appears to be working fie. Thanks |
chrisrink10
force-pushed
the
feature/use-module
branch
from
b4d5bb9 to
42534f1
Compare
Yes and thank you for the reminder. I have incorporated your suggestions as written. |
Hi, could you please review patch to fix a likely regression issue where the testrunner couldn't handle relative paths in `sys.path`. It fixes #1204. This is likely a regression from #1176, where the update to the new module loading system caused relative paths to never match a fully qualified module name. The patch now expands relative paths to their absolute form before comparing them to the module path. This issue was breaking the vanilla `basilisp test` command, as it adds an empty string (`""`) entry to `sys.path`, which is interpreted as `"."`. Since this is a relative path, it wasn’t handled by the logic, causing the project directory not to be recognized as the root of the Basilisp test modules. I’ve added a test that invokes `basilisp test` directly as a subprocess. Unfortunately, I couldn’t find a way to control the pytester `sys.path` entries with the precise control required for this test. Any counter suggestions are most welcome. Thanks --------- Co-authored-by: ikappaki <[email protected]>
Possible alternative solution to #1165 and #1155
This PR changes the Python
importlibmachinery for Basilisp to simply focus on importing modules, without getting itself too mixed up in the business of figuring out what a namespace is actually called from the module name. Whenimportlib.import_moduleis called, Basilisp just figures out the location of the module on disk and starts loading it up as if it were a Basilisp module. It sets the newly introducedbasilisp.core/*import-module*dynamic Var to the value of the importing module and lets the Basilisp runtime fetch that value down and attach it to a new namespace if and when it is appropriate to do so.There may be some unintended side effects here for strange cases, so noting those here for now:
import.import_lib(excluding recursive calls for other imports or requires), a module could be attached to multiple namespaces. I'm not sure if that's actually an issue, but it's certainly bizarre and could lead to some unintended consequences. That being said, I don't really think it is a priority for the project to support this style of bizarre metaprogramming, so that may be fine.