-
Notifications
You must be signed in to change notification settings - Fork 152
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
Improves more optional module imports #367
Conversation
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.
Hi @mattpopovich , this also looks GREAT to me, but let's merge #366 first!
Agreed. Let me know if you have any thoughts/suggestions on how to fix that circular reference... I didn't look into it too closely yet. |
Hi @mattpopovich , that's my bad, I mistakenly put the following two functions in the current position, and these two dependencies will cause the circular reference calls. I think it should be better to move I'll submit a PR to resolve this issue. |
006654e
to
9f1e376
Compare
Hi @mattpopovich , After #369 is merged, I guess the circular import error will be resolved, I rebase manually this branch to the main, let's test these on the CI again. Because of my manual rebase, the code now has some conflicts with your branch, please use the following command to synchronize with the current branch: git pull --rebase origin improve-more-imports |
Codecov Report
@@ Coverage Diff @@
## main #367 +/- ##
=======================================
Coverage 98.58% 98.58%
=======================================
Files 11 11
Lines 778 778
=======================================
Hits 767 767
Misses 11 11
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
ff6bc96
to
a295096
Compare
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.
Hi @mattpopovich , the overall looks great to me, I only add some comments about where to place the decorator, let me know if you have more concerns here.
And after #366 is merged, I only cherry-pick one commit from your commits history (the others have been merged to the main branch), and now you should use following command to synchronize with the current branch:
git pull --rebase origin improve-more-imports
@zhiqwang your requested changes have been made, CI looks good 👍 |
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.
Thanks @mattpopovich again, this looks great to me!
Builds on #366 to remove more
ImportError
s. Not sure if you wanted these done as well so I made it into an additional PR.There are a few
ImportError
s remaining, it wasn't clear to me if I should remove those as well or keep them. I'll leave that to you to decide.