Skip to content

Hotfix/calc refactor - #832

Closed
adampalay wants to merge 4 commits into
rc/2013-08-27from
hotfix/calc-refactor
Closed

Hotfix/calc refactor#832
adampalay wants to merge 4 commits into
rc/2013-08-27from
hotfix/calc-refactor

Conversation

@adampalay

Copy link
Copy Markdown
Contributor

fixes issues we've had with sandboxing on prod and edge by restructuring the calc package.

@nedbat
@cpennington

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor

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."

@cpennington

Copy link
Copy Markdown
Contributor

I'd also move calcfunctions.py to calc/functions.py, rather than calc/calcfunctions.py

@nedbat

nedbat commented Aug 30, 2013

Copy link
Copy Markdown
Contributor

Good point about calcfunctions -> functions.

@nedbat

nedbat commented Aug 30, 2013

Copy link
Copy Markdown
Contributor

👍 tested on my sandboxed dev machine.

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

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'

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor Author

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?

@cpennington

Copy link
Copy Markdown
Contributor

👍

@adampalay adampalay mentioned this pull request Aug 30, 2013
@adampalay adampalay closed this Aug 30, 2013
@adampalay adampalay mentioned this pull request Aug 30, 2013
jenkins-ks pushed a commit to nttks/edx-platform that referenced this pull request Jun 3, 2016
Sujeet1379 pushed a commit to chandrudev/edx-platform that referenced this pull request Nov 17, 2022
DanielVZ96 referenced this pull request in open-craft/openedx-platform Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants