-
Notifications
You must be signed in to change notification settings - Fork 16
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 bug with function name replacement #23
Conversation
Issue to clean up uneeded imports: #24 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
looks like it should work correctly with nested qualified names, but a test for that may be warranted.
What do you mean exactly, could you give an example? |
Something like this:
|
I don't understand. Do you mean a case when there is more than 1 dot in the qualified name? |
Sorry for not being clear. In my example I was indeed referring to the hypothetical case when If that case is supported and tested, then awesome. |
Before this PR, TorchFix in standalone mode crashes on
With this PR it works fine, replacing the code with
Note that the replacement code still imports no longer needed
ger
, but that can be caught and fixed by existing linters like ruff.Nevertheless, I'll create an issue to clean this up later in TorchFix.