Skip to content
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.

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

Fix ._orig access for submodules for MF extensions #1174

Merged
merged 4 commits into from
Nov 17, 2022
Merged

Conversation

romain-intel
Copy link
Contributor

This addresses an issue where submodules like mymodule._orig.submodule would not load properly.

This will properly load things like from .foofa import xyz from a module or sub-module in the original location but currently does not work if the import path is absolute.

This addresses an issue where submodules like `mymodule._orig.submodule` would
not load properly.

This will properly load things like `from .foofa import xyz` from a module or
sub-module in the original location but currently does not work if the
import path is absolute.
Copy link

@roofurmston roofurmston left a comment

Choose a reason for hiding this comment

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

Not an expert with the inner working of the Python import system, so learnt a lot from the PR. :)

the PR looks good to me.

The one thing I wonder is if the changes merit some additional tests. You think it is worth adding some? Would be happy to have a stab at adding some, if you think it is worthwhile.

@romain-intel
Copy link
Contributor Author

Yep. I think tests would be nice. THere are some unit tests that test the extension mechanism. We can just add an extension that overrides something simple and see if we can access _orig. Would you be able to do that? Happy to provide pointers. And don't feel obligated, if you don't have time, I can do it too :).

@roofurmston
Copy link

Yep. I think tests would be nice. THere are some unit tests that test the extension mechanism. We can just add an extension that overrides something simple and see if we can access _orig. Would you be able to do that? Happy to provide pointers. And don't feel obligated, if you don't have time, I can do it too :).

Yeah, would be very happy to have a go at it :)

I'll have a crack at it next week.

@tfurmston
Copy link
Contributor

OK, so I've had a go at adding a test case. I picked the most innocuous looking module to extend.

The attempt is here, if you want to have a look - #1192. Still waiting for the tests to start in CI, but I checked the ExtensionsTest locally and it went through fine.

One thing I wasn't sure about, will the frameworks module be extended just for the ExtensionsTest test? Or will it be for all test casses?

@romain-intel romain-intel marked this pull request as ready for review November 15, 2022 08:26
@nflx-mf-bot
Copy link
Collaborator

Testing[291] @ 4665d5a

@nflx-mf-bot
Copy link
Collaborator

Testing[291] @ 204f823

@nflx-mf-bot
Copy link
Collaborator

Testing[291] @ 204f823 PASSED

@savingoyal savingoyal merged commit e7e91ed into master Nov 17, 2022
@savingoyal savingoyal deleted the fix/orig_modules branch November 17, 2022 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants