-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Hotfix/calc refactor #832
Hotfix/calc refactor #832
Conversation
@@ -0,0 +1 @@ | |||
from calc import * |
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.
If it were me, I would have just moved calc.py
to calc/__init__.py
. I think @nedbat would disagree, though.
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.
Yes, this was my suggestion. As a compromise, I'd add a comment here explaining why: "Ideally, we wouldn't need to pull all the calc symbols up here, but courses were using 'import calc', so this is here to keep them working."
I'd also move |
Good point about calcfunctions -> functions. |
👍 tested on my sandboxed dev machine. |
@@ -9,7 +9,7 @@ | |||
import numbers | |||
import numpy | |||
import scipy.constants | |||
import calcfunctions | |||
import functions |
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.
I tend to prefer full imports: import calc.functions
, just for clarity
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.
Adding this seems to add some sort of circular import error. If I am a course author, and I run
'import' calc, it invokes 'from calc import *' which calls import 'calc.functions'.
Or: 'import calc.functions' -> 'from calc import *' -> 'import calc.functions'
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.
Oh. Hrm. Ok, let's just leave it like this for now, then.
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.
I'll rebase, and then good to go?
👍 |
fixes issues we've had with sandboxing on prod and edge by restructuring the calc package.
@nedbat
@cpennington