build: Restructure repo & rename to xblocks-core#244
build: Restructure repo & rename to xblocks-core#244kdmccormick wants to merge 10 commits intomainfrom
Conversation
cbb0348 to
e92b6ed
Compare
| @@ -14,7 +14,7 @@ <h3 class="hd hd-2">{{ display_name|escape }}</h3> | |||
| data-block-id='{{ block_id|escape }}' | |||
| data-course-id='{{ course_id|escape }}' | |||
| tabindex="-1" | |||
| data-package-name="xblocks-contrib"> | |||
| data-package-name="xblocks-core"> | |||
There was a problem hiding this comment.
The only mention of this is in this file and I didn't see any mention of this in the openedx-platform history so I have no idea.
There was a problem hiding this comment.
We added it for testing purpose to differentiate between BuiltIn and Extracted Block at brower.
- renaming with repo sounds good.
- We can remove it after removal of BuiltIn blocks, removing now seems alright as well as we are pretty much done with the major testing.
86c65f3 to
5fbbf35
Compare
a70e618 to
a91002d
Compare
bd1ae1d to
76dd6bc
Compare
| runs-on: ubuntu-latest | ||
| strategy: | ||
| fail-fast: true | ||
| fail-fast: false |
There was a problem hiding this comment.
| fail-fast: false | |
| fail-fast: true |
Note to self: Undo this before merging
fe1f184 to
7a9efbf
Compare
|
@feanil Tests all pass on this locally. I'm still manually testing and waiting on GHA to catch up, but feel free to take a look if you have time. You can review commit-by-commit. |
9adca8d to
2347fe2
Compare
With skipsdist=true globally, usedevelop=True alone doesn't install the package in the docs env. Add explicit pip install -e . to match the workaround already present in [testenv], so xblocks_core is importable during sphinx-apidoc autodoc. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2347fe2 to
fbe6c1c
Compare
The restructure changed `pylint src` to cover all packages under src/, including packages whose tests were never run through this pylint setup before (they came from other repos). Fix the resulting failures: - Remove unused imports in test_poll.py and test_pdf.py - Remove unused variable scope_ids in test_pdf.py - Add missing module/class docstrings in test_annotatable.py, test_html.py, prog2.py, constant.py - Disable protected-access at class level in test_annotatable.py (tests legitimately access private methods) - Disable no-member false positive on response.body in test_annotatable.py - Fix arguments-renamed W0237 in test_html.py render() (view→view_name) - Suppress abstract-method and unused-argument in test_html.py stubs - Convert dict() constructor calls to dict literals in test_html.py - Remove now-useless pylint disable comments in test_discussion.py and video.py (newer pylint no longer triggers those warnings) - Fix how test_annotations.py uses supressions - Fix a suppression in test_html Co-Authored-By: Kyle D McCormick <kyle@axim.org> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
These webpack configs use const { merge } = require('webpack-merge')
which is webpack-merge v5 API. However webpack-merge was only present
as a transitive dep at v4 (pulled in by karma-webpack ^0.13), causing
TypeError: merge is not a function at build time.
Adding it as a direct devDependency at ^5 causes npm to install v5
under each workspace's node_modules, matching what the configs require.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The import path `xblock_poll` is taken by the *newer* PollBlock, developed and maintained by OpenCraft: https://github.com/open-craft/xblock-poll We don't want to conflict with that block, so we've made the old formerly- built-in block's path xblock_poll_question, which matches its entrtypoint and XML tag, <poll_question>.
fbe6c1c to
ba3dc09
Compare
I made the app changes, and Claude fixed the mocks. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
feanil
left a comment
There was a problem hiding this comment.
I tried to do as thorough a review as I could on this big change, but there are a lot of moving parts so the openedx-platform testing will be critical for this. That said, a not for other reviewers is that the consolidated diff makes reading the translations changes a bit confusing since it makes it seem like translations conf files were move around in between blocks. But looking at the commit diffs it's more obvious what's going on.
| @@ -14,7 +14,7 @@ <h3 class="hd hd-2">{{ display_name|escape }}</h3> | |||
| data-block-id='{{ block_id|escape }}' | |||
| data-course-id='{{ course_id|escape }}' | |||
| tabindex="-1" | |||
| data-package-name="xblocks-contrib"> | |||
| data-package-name="xblocks-core"> | |||
There was a problem hiding this comment.
The only mention of this is in this file and I didn't see any mention of this in the openedx-platform history so I have no idea.
|
Thanks @feanil ! Per my latest testing, this is working fine on
I'll hold off until merging until I can fix |
Because of the way the repo used to be structured (xblock_contrib/<blah>), we had different paths for dev assets (<blah>/public/<asset>) and prod assets (public/<asset>) relative to the block. Now that we have a flat structure (src/<blah>_block), dev and prod assets share the same path (public/<asset>) relative to the block, so we can remove a bunch of unnecessary and bug-prone code which made the assets different between dev and prod. This commit fixes an issue where prod asset paths were wrong.
farhan
left a comment
There was a problem hiding this comment.
LGTM!
Skipped translations code review as it needs to work on it later on.
|
|
||
| * In Development: We're building and testing this block. | ||
| * Ready to Use: You can try this on your site using the Waffle flag. | ||
| * Done: The built-in block has been removed. The setup.py entrypoint has been removed from edx-platform and added to xblock-contrib. |
There was a problem hiding this comment.
| * Done: The built-in block has been removed. The setup.py entrypoint has been removed from edx-platform and added to xblock-core. |

This applies both of the following:
xblocks-core#149Additionally, it standardizes the name of the module where the block is defined to be
block.py. So, for every block, both of these are valid:Platform PR:
AI
It did the first pass at restructuring, I've been fixing things since then. All code reviewed by hand.
Feanil used Claude to fix a few more of the build issues. You can see credit broken down by commit.
Testing
I smoke-tested the demo course using the sandbox on openedx/openedx-platform#38473
I smoke-tested the demo course imported into a V2 library on my local machine.
Merge checklist
Check off if complete or not applicable: