Fix load_module wrapping #3948
Fix load_module wrapping #3948
Conversation
|
@cduck I hope this might be the last of these - if you have the chance I'd appreciate your input! |
balopat
force-pushed
the
fix_submodule_searchpath
branch
from
0eacf73
to
b8fdb68
Compare
cirq/_compat.py
Outdated
| sys.modules[self.old_module_name] = sys.modules[self.new_module_name] | ||
| return sys.modules[self.old_module_name] | ||
| method(self.new_module_name) | ||
| assert self.new_module_name in sys.modules |
There was a problem hiding this comment.
added, with a link to the specs where the expectation is coming from
cirq/_compat_test.py
Outdated
| @@ -679,19 +680,69 @@ def exec_module(self, module: ModuleType) -> None: | |||
| assert 'new' not in sys.modules | |||
|
|
|||
|
|
|||
| def test_loader_wrappers(): | |||
| def test_loader_create_module(): | |||
| class EmptyLoader: | |||
There was a problem hiding this comment.
Add comment why this purposefully does not subclass importlib.abc.Loader.
There was a problem hiding this comment.
oh, good catch, actually, it can be a subclass of Loader, I added that in.
balopat
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
CirqBot
removed
automerge
Hmmm sorry I didn't catch this before merging. We don't have explicit guidance, so probably the Google Style Guide applies here: https://google.github.io/styleguide/pyguide.html#310-strings Be consistent with your choice of string quote character within a file. Pick ' or " and stick with it. It is okay to use the other quote character on a string to avoid the need to \ escape within the string. Yes: Python('Why are you hiding your eyes?')
Gollum("I'm scared of lint errors.")
Narrator('"Good!" thought a happy Python reviewer.')No: Python("Why are you hiding your eyes?")
Gollum('The lint. It burns. It burns us.')
Gollum("Always the great lint. Watching. Watching.")Prefer """ for multi-line strings rather than '''. Projects may choose to use ''' for all non-docstring multi-line strings if and only if they also use ' for regular strings. Docstrings must use """ regardless. I guess it would be useful to add this to our style guide explicitly and maybe also do a check across the codebase. |
|
For old type (g3) module loaders, the resolution wasn't working, breaking internal par tests.
This makes
DeprecatedModuleLoader.load_modulework similarly toexec_modulethat it ensures that when the old module is loaded, both the new and old names are pointing to the same module in the module cache.cc @cduck - this should be straightforward. The reason it didn't come up earlier, as I haven't tested on g3 since my change in #3917 (comment) - I think that more principled rewiring surfaced this bug :)