Skip to content

build: Restructure repo & rename to xblocks-core#244

Open
kdmccormick wants to merge 10 commits intomainfrom
kdmccormick/xblocks-core
Open

build: Restructure repo & rename to xblocks-core#244
kdmccormick wants to merge 10 commits intomainfrom
kdmccormick/xblocks-core

Conversation

@kdmccormick
Copy link
Copy Markdown
Member

@kdmccormick kdmccormick commented Apr 29, 2026

This applies both of the following:

Additionally, 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:

from xblock_foo import FooBlock  # preferred externally
from xblock_foo.block import FooBlock  # used internally, OK if used externally

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:

  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Fixup commits are squashed away
  • Unit tests added/updated
  • Manual testing instructions provided
  • Noted any: Concerns, dependencies, migration issues, deadlines, tickets

@kdmccormick kdmccormick force-pushed the kdmccormick/xblocks-core branch from cbb0348 to e92b6ed Compare April 29, 2026 13:55
@@ -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">
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this matter?

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.

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.

Comment thread src/xblock_word_cloud/.tx/config
runs-on: ubuntu-latest
strategy:
fail-fast: true
fail-fast: false
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fail-fast: false
fail-fast: true

Note to self: Undo this before merging

@kdmccormick kdmccormick force-pushed the kdmccormick/xblocks-core branch 2 times, most recently from fe1f184 to 7a9efbf Compare May 4, 2026 20:04
@kdmccormick kdmccormick marked this pull request as ready for review May 4, 2026 20:06
@kdmccormick kdmccormick requested a review from feanil May 4, 2026 20:07
@kdmccormick
Copy link
Copy Markdown
Member Author

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

@kdmccormick kdmccormick force-pushed the kdmccormick/xblocks-core branch from 9adca8d to 2347fe2 Compare May 5, 2026 16:28
kdmccormick and others added 3 commits May 5, 2026 12:30
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>
@kdmccormick kdmccormick force-pushed the kdmccormick/xblocks-core branch from 2347fe2 to fbe6c1c Compare May 5, 2026 16:31
feanil and others added 4 commits May 5, 2026 12:32
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>.
@kdmccormick kdmccormick force-pushed the kdmccormick/xblocks-core branch from fbe6c1c to ba3dc09 Compare May 5, 2026 16:34
I made the app changes, and Claude fixed the mocks.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

@kdmccormick
Copy link
Copy Markdown
Member Author

Thanks @feanil !

Per my latest testing, this is working fine on tutor dev, but failing on tutor local:

Screenshot 2026-05-07 at 11 18 24 AM

I'll hold off until merging until I can fix local.

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

2 participants