Skip to content

Cohorted courseware - #5942

Merged
andy-armstrong merged 6 commits into
masterfrom
cohorted-courseware
Dec 8, 2014
Merged

Cohorted courseware#5942
andy-armstrong merged 6 commits into
masterfrom
cohorted-courseware

Conversation

@andy-armstrong

Copy link
Copy Markdown
Contributor

TNL-648

@jimabramson

Copy link
Copy Markdown

rebased

@andy-armstrong
andy-armstrong force-pushed the cohorted-courseware branch 3 times, most recently from e7dd8f4 to 8e020e0 Compare November 19, 2014 03:32
@jimabramson

Copy link
Copy Markdown

@jzoldak any chance of getting testeng's review of the 3 files changed in common/test/db_cache in the next 24h or so? we are looking to merge this branch to master asap to reduce any further drift.

@jzoldak

jzoldak commented Nov 25, 2014

Copy link
Copy Markdown
Contributor

👍 for the test db schema changes.

@jimabramson

Copy link
Copy Markdown

thanks @jzoldak

@andy-armstrong
andy-armstrong force-pushed the cohorted-courseware branch 3 times, most recently from 194ece6 to 3fd07d9 Compare December 4, 2014 22:16
@andy-armstrong

Copy link
Copy Markdown
Contributor Author

@cahrens @jimabramson I've rebased this branch and squashed in the change to hide the user_partitions advanced property again. We now have a clean build so I think we just need to handle the migration issue and send the email, and then we can merge this back to master. Please re-review as you see fit.

@cahrens

cahrens commented Dec 5, 2014

Copy link
Copy Markdown

@andy-armstrong Needs a rebase. I'd also recommend a CHANGELOG entry (stating that the implementation is not complete).

@andy-armstrong

Copy link
Copy Markdown
Contributor Author

Wow, it was fully rebased last night! I guess this is why we need to get this merged asap. Good suggestion about the CHANGELOG entry.

andy-armstrong and others added 3 commits December 5, 2014 09:52
JIRA: TNL-710

IMPORTANT: this commit converts the course_groups
package to using migrations.  When deploying to an
existing openedx instance, migration 0001 may fail
with an error indicating that the CourseUserGroup
table already exists.  If this happens, running
the 0001 migration first, with the --fake option,
is recommended.  After performing this step,
remaining migrations should work as expected.
@nedbat

nedbat commented Dec 8, 2014

Copy link
Copy Markdown
Contributor

I don't like naming the new common directory "openedx". This makes it look like Open edX is a subset of the code in this repo, when it is actually a superset. Can we talk about picking a different name? "apps"? "lib"? "common2"?

@andy-armstrong

Copy link
Copy Markdown
Contributor Author

@nedbat The name was originally 'edx' which @cpennington suggested, but then @singingwolfboy suggested that it should be 'openedx' as this was not unique to edX. I agree with you that it doesn't encompass all of openedx, and it will never become that, so I can see that the name is less than ideal.

I'm open to a different name if we can come up with an acceptable one (but most definitely not 'common2' or 'common-new'... I hate names that become obsolete). How about "shared"? @cahrens @jimabramson, do you guys have opinions?

@jimabramson

Copy link
Copy Markdown

I was just about to suggest "shared" as well. Or perhaps lib/shared.

On Dec 8, 2014, at 8:02 AM, Andy Armstrong notifications@github.com wrote:

@nedbat The name was originally 'edx' which @cpennington suggested, but then @singingwolfboy suggested that it should be 'openedx' as this was not unique to edX. I agree with you that it doesn't encompass all of openedx, and it will never become that, so I can see that the name is less than ideal.

I'm open to a different name if we can come up with an acceptable one (but most definitely not 'common2' or 'common-new'... I hate names that become obsolete). How about "shared"? @cahrens @jimabramson, do you guys have opinions?


Reply to this email directly or view it on GitHub.

@cpennington

Copy link
Copy Markdown
Contributor

The reason I suggested edx originally was so that imports would be something like from edx.djangoapps.foobar import baz. That also gives us a path forward toward making edx a python namespace package so that we can install many of our subpackages into it (even when they're in separate repos). from shared.djangoapps.foobar import baz is significantly worse on both fronts.

avoid errors when forward-migrating course_groups after south conversion
@jimabramson

Copy link
Copy Markdown

based on that logic I agree with using "edx", if @singingwolfboy + @nedbat
are comfortable with it.

On Mon, Dec 8, 2014 at 8:50 AM, Calen Pennington notifications@github.com
wrote:

The reason I suggested edx originally was so that imports would be
something like from edx.djangoapps.foobar import baz. That also gives us
a path forward toward making edx a python namespace package so that we
can install many of our subpackages into it (even when they're in separate
repos). from shared.djangoapps.foobar import baz is significantly worse
on both fronts.


Reply to this email directly or view it on GitHub
https://github.com/edx/edx-platform/pull/5942#issuecomment-66117744.

@nedbat

nedbat commented Dec 8, 2014

Copy link
Copy Markdown
Contributor

"edx_shared"? I'm not sure a namespace package is worth it.

@andy-armstrong

Copy link
Copy Markdown
Contributor Author

Thanks @cpennington. I had forgotten that you were thinking of it as a namespace, and not as a package name. I wonder if we still need an extra layer in there to distinguish these apps from all the other edx apps. It also implies that eventually everything will live in this 'edx' directory since by definition everything here is in the edx namespace. Would you see lms and cms eventually moving in here too?

One avenue of thinking I was exploring on my way to work is what should this be called once we have the LMS/CMS split. If we make an edx-cms (or edx-studio) repo and have a clean separation, then what's left? Would we want to split out lms too, and then edx-platform really does become the platform? Or would we leave LMS in platform? With that way of looking at things, it might make sense to name this new folder 'platform' with an eye to it being all of the platform code. Does it make sense that these are edX platform Django apps, compared to LMS or CMS Django apps?

One final thought: the long import names seem a little odd compared to all the other imports in the code:

from edx.core.djangoapps.user_api.models import UserPreference

Do you think that having this very explicit namespacing is the right way to go? Should all our code look like this?

@andy-armstrong

Copy link
Copy Markdown
Contributor Author

@cahrens @jimabramson I'm going to merge this back to master in a couple of hours, once it is clear that the namespace name is going to stick. I have the email about pip install ready to go.

andy-armstrong added a commit that referenced this pull request Dec 8, 2014
@andy-armstrong
andy-armstrong merged commit 2b0f959 into master Dec 8, 2014
kdmccormick added a commit that referenced this pull request Jun 10, 2025
This directory was created as part of an
idea [1] to create top-level openedx namespace which
every Open edX package would nest itself under.
The README was written with that in mind.

That never panned out, so openedx is really just
another "common" directory. Non-common code
should not be added to it. This update clarifies that.

[1] #5942 (comment)
marlonkeating pushed a commit that referenced this pull request Jul 15, 2025
This directory was created as part of an
idea [1] to create top-level openedx namespace which
every Open edX package would nest itself under.
The README was written with that in mind.

That never panned out, so openedx is really just
another "common" directory. Non-common code
should not be added to it. This update clarifies that.

[1] #5942 (comment)
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.

6 participants