search

jazkarta / edx-platform

the edX learning management system (LMS) and course authoring tool, Studio
http://code.edx.org/
GNU Affero General Public License v3.0
1 stars 0 forks source link

Address Ned's comments (minor, I believe) #49

Closed pdpinch closed 9 years ago

pdpinch commented 9 years ago

If any of these turn out to be signficant issues, we can break them out into new issues:

https://github.com/edx/edx-platform/pull/6636#discussion-diff-25201906 This import is redundant, or the previous one is.

https://github.com/edx/edx-platform/pull/6636#discussion-diff-25202636 "pfm": the old name is poking through! :)

https://github.com/edx/edx-platform/pull/6636#discussion_r25203342 Can't this just be shortened to return _CCX_CONTEXT.ccx ?

https://github.com/edx/edx-platform/pull/6636#discussion-diff-25203627 Seems like there are a few places that examine the feature flag, and then get_current_ccx. It might help to have a single function that combines the two.

https://github.com/edx/edx-platform/pull/6636#discussion-diff-25203981 A comment here would be a nice touch. :)

https://github.com/edx/edx-platform/pull/6636#discussion-diff-25204047 s/individual due dates/custom courses/

https://github.com/edx/edx-platform/pull/6636#discussion_r25205476 You probably wanted to add a close-paren at the end of the format string.

cguardia commented 9 years ago

This issues have been addressed by https://github.com/jazkarta/edx-platform/commit/e46b8b7d3389726441fe1a6367eda0de24b1a9cb

pdpinch commented 9 years ago

@pwilkins can you close this after you've done regression tests?

pwilkins commented 9 years ago

@pdpinch I'll close this on the basis of my blackbox testing. If I don't see any elephants, we assume that the magic elephant repellent worked.