feat!: rename LibraryCollectionLocator.collection_id to collection_code#434
feat!: rename LibraryCollectionLocator.collection_id to collection_code#434kdmccormick wants to merge 1 commit intomasterfrom
Conversation
BREAKING CHANGE: `LibraryCollectionLocator.collection_id` is renamed to `LibraryCollectionLocator.collection_code`. There is a readonly `collection_id`` property for backwards compatibility, but calls to the constructor which use the `collection_id=` kwarg must be updated to `collection_code=`. Part of: openedx/openedx-core#322
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #434 +/- ##
=======================================
Coverage 94.13% 94.13%
=======================================
Files 31 31
Lines 3068 3071 +3
Branches 191 191
=======================================
+ Hits 2888 2891 +3
Misses 155 155
Partials 25 25
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
This looks good, but there are a lot of other similar things I'd like to fix. Is your idea to do them all incrementally, releasing many breaking versions of opaque-keys ?
LibraryContainerLocator.container_type -> container_type_code
LibraryContainerLocator.container_id -> container_code
LibraryUsageLocatorV2.usage_id -> usage_code
LibraryLocatorV2.org -> org_code
LibraryLocatorV2.slug -> library_code
and that's just for V2 libraries.
| """ | ||
| return self.lib_key.org | ||
|
|
||
| @property |
There was a problem hiding this comment.
Can we add the @deprecated decorator? It's technically a Python 3.13 feature but you can get it via from typing_extensions import deprecated in 3.12.
There was a problem hiding this comment.
good idea. i'll do that when I open the broader renaming PR
| There is a readonly ``collection_id`` property for backwards compatibility, but calls to the constructor | ||
| which use the ``collection_id=`` kwarg must be updated to ``collection_code=``. |
There was a problem hiding this comment.
Why can't we make the constructor accept a backwards compatible kwarg, so this isn't a breaking change? Because it's difficult, or because we want to force a change sooner and avoid having an inconsistent state for a long time?
|
Yeah, maybe it's better to do all these changes in one PR. I was worried about the headache of so having many breaking change at once, but I like the idea of supporting old kwarg names, which would mean we could avoid having any breaking changes for now. |
2 participants
BREAKING CHANGE:
LibraryCollectionLocator.collection_idis renamed toLibraryCollectionLocator.collection_code. There is a readonlycollection_id`` property for backwards compatibility, but calls to the constructor which use thecollection_id=kwarg must be updated tocollection_code=`.Part of: openedx/openedx-core#322
Related openedx-core PR: openedx/openedx-core#504
Related openedx-platform PR: openedx/openedx-platform#38214