Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge openedx/xblock-utils library into openedx/XBlock #675

Closed
6 tasks done
kdmccormick opened this issue May 2, 2023 · 10 comments · Fixed by #669 or #672
Closed
6 tasks done

Merge openedx/xblock-utils library into openedx/XBlock #675

kdmccormick opened this issue May 2, 2023 · 10 comments · Fixed by #669 or #672
Assignees
Labels
code health Proactive technical investment via refactorings, removals, etc. documentation Relates to documentation improvements enhancement Relates to new features or improvements to existing features

Comments

@kdmccormick
Copy link
Member

kdmccormick commented May 2, 2023

Background

Pros of having this as a separate package

  • Code separation: XBlock's core cannot depend on these utilities
    • However: importlinter could be used instead.
  • XBlock doesn't depend on Django
    • However: I can't find the ADR, but somewhere we decided that Django should be assumed for XBlocks, since enforcing Django-agnosticness wasn't yielding any value.

Cons of having this as a separate package

  • More maintenance overhead. We can't assume XBlock authors use xblock-utils, so we must maintain XBlock as a proper interface on its own. For example, moving off pkg_resources would be simpler if every block just used the ResourcesLoader interface that xblock-utils provides.
  • Harder to document: Two sets of documentation & common patterns need to be maintained -- docs for using XBlock without xblockutils, and docs for xblockutils itself.
  • More choices for XBlock developers. "Should I use xblockutils or not??"
  • More setup steps for XBlock developers.
  • Indirection: the system is harder for developers to understand when there's extra layers.
  • Maintenance difficulty:

Acceptance Criteria

Tasks

@bradenmacdonald
Copy link
Contributor

@kdmccormick Thanks for bringing this up. I think it's an excellent idea to merge this into XBlock. I think I can answer some historical questions for you.

understand why the two libraries were separate in the first place

I believe xblock-utils was created by OpenCraft. At the time we were creating tons of XBlocks, so it was a repo that held common code/patterns for use in our various XBlocks. I suspect that we kept it separate so we could iterate on it rapidly, as at the time it took too long to get things merged into Open edX core repos like XBlock or edx-platform. At some point, other XBlock developers and eventually even core platform XBlocks started using the xblock-utils library. Over time, this awkward split was never really properly re-evaluated as you're doing now.

XBlock doesn't depend on Django
I can't find the ADR, but somewhere we decided that Django should be assumed for XBlocks, since enforcing Django-agnosticness wasn't yielding any value.

AFAIK XBlock was originally intended to be "portable" to other LMSs, but since it is python based, it can only really work with other python-based LMSs, and the only one that it ever actually worked with or was designed to work with was Google Course Builder via coursebuilder_xblock_module. Since Course Builder was not Django-based, XBlock had to be Django-free. However, that project didn't last very long and nobody has been using XBlocks in anything other than Open edX for many years at this point, to the best of my knowledge. So I don't see any reason to avoid assuming Django in XBlock code anymore.

@kdmccormick kdmccormick added code health Proactive technical investment via refactorings, removals, etc. enhancement Relates to new features or improvements to existing features documentation Relates to documentation improvements and removed discovery Pre-work to determine if an idea is feasible labels Jun 2, 2023
@kdmccormick kdmccormick changed the title Should this library be merged into openedx/XBlock? Merge this library be merged into openedx/XBlock Jun 2, 2023
@kdmccormick kdmccormick changed the title Merge this library be merged into openedx/XBlock Merge this library into openedx/XBlock Jun 2, 2023
@kdmccormick
Copy link
Member Author

@bradenmacdonald That's really helpful context, thanks! I'm convinced at this point that we should merge them; I've updated this ticket's AC capture that.

@farhan
Copy link
Contributor

farhan commented Sep 14, 2023

Here is the XBlock repo PR to merge this library into it
#669

@farhan
Copy link
Contributor

farhan commented Sep 14, 2023

Progress update:

  • The xblock-utils library has been moved with unit test cases.
  • Integration tests of the library have been dropped because of the deprecation of bok-choy library.

Below 3 tasks are pending:

  1. Passing tox test cases on github-actions. They are passing locally. Xblock repo run test cases on pip package rather than project files. On GitHub actions pip package is missing some data files that are required in test cases. Currently looking into it.
  2. After removing of bok-choy test cases, code coverage falls below the required target which needs to write missing test cases. Majorly for studio_editable.py file
  3. Need to move the documentation as well (either in readme or sphinx docs)
    https://github.com/openedx/xblock-utils#more-documentation

@feanil We can cover points 2 and 3 in separate stories. Should I create one for them?

@farhan
Copy link
Contributor

farhan commented Sep 14, 2023

Passing tox test cases on github-actions. They are passing locally. Xblock repo run test cases on pip package rather than project files. On GitHub actions pip package is missing some data files that are required in test cases. Currently looking into it.

Above point 1 has been fixed by updating the setuptools

56c2a8b

@feanil
Copy link
Contributor

feanil commented Sep 14, 2023

@farhan I think it's fine to create separate stories for Points 2 and 3. For the documentation, you should move it to be a new sphinxs docs under XBlock/docs.

@farhan
Copy link
Contributor

farhan commented Sep 21, 2023

PR for docs shifting:
#672

@farhan
Copy link
Contributor

farhan commented Sep 27, 2023

I am deprecating the xblock-utils repository within the scope of this ticket

Here is the PR: openedx-unsupported/xblock-utils#231

@feanil feanil changed the title Merge this library into openedx/XBlock Merge openedx/xblock-utils library into openedx/XBlock Oct 5, 2023
@feanil feanil transferred this issue from openedx-unsupported/xblock-utils Oct 5, 2023
@feanil
Copy link
Contributor

feanil commented Oct 5, 2023

The xblock-utils repo has been archived and moved to the openedx-unsupported org. I moved this issue to the XBlock repo so that it can retain the history of this move.

@feanil feanil closed this as completed Oct 16, 2023
kdmccormick added a commit to kdmccormick/xblock-poll that referenced this issue Jan 9, 2024
Context: openedx/XBlock#675

This fixes some edx-platform warnings:

    /openedx/venv/lib/python3.8/site-packages/poll/poll.py:40:
    DeprecatedPackageWarning: Please use import xblock.utils.publish_event instead of
    xblockutils.publish_event because the 'xblock-utils' package has been deprecated and
    migrated to within 'xblock' package.

Bump from 1.13.0 to 1.13.1
@kdmccormick
Copy link
Member Author

kdmccormick commented Jan 9, 2024

Some follow-up: open-craft/xblock-poll#127

kdmccormick added a commit to kdmccormick/xblock-poll that referenced this issue Jan 9, 2024
Context: openedx/XBlock#675

This fixes some edx-platform warnings:

    /openedx/venv/lib/python3.8/site-packages/poll/poll.py:40:
    DeprecatedPackageWarning: Please use import xblock.utils.publish_event instead of
    xblockutils.publish_event because the 'xblock-utils' package has been deprecated and
    migrated to within 'xblock' package.

Uses a try-catch for backwards compatibility with pre-Quince versions
of xblock and xblockutils.

Bump from 1.13.0 to 1.13.1
kdmccormick added a commit to kdmccormick/xblock-poll that referenced this issue Jan 9, 2024
Context: openedx/XBlock#675

This fixes some edx-platform warnings:

    /openedx/venv/lib/python3.8/site-packages/poll/poll.py:40:
    DeprecatedPackageWarning: Please use import xblock.utils.publish_event instead of
    xblockutils.publish_event because the 'xblock-utils' package has been deprecated and
    migrated to within 'xblock' package.

Uses a try-catch for backwards compatibility with pre-Quince versions
of xblock and xblockutils.

Bump from 1.13.0 to 1.13.1
kdmccormick added a commit to kdmccormick/xblock-poll that referenced this issue Jan 9, 2024
Context: openedx/XBlock#675

This fixes some edx-platform warnings:

    /openedx/venv/lib/python3.8/site-packages/poll/poll.py:40:
    DeprecatedPackageWarning: Please use import xblock.utils.publish_event instead of
    xblockutils.publish_event because the 'xblock-utils' package has been deprecated and
    migrated to within 'xblock' package.

Uses a try-catch for backwards compatibility with pre-Quince versions
of xblock and xblockutils.

Bump from 1.13.0 to 1.13.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health Proactive technical investment via refactorings, removals, etc. documentation Relates to documentation improvements enhancement Relates to new features or improvements to existing features
Projects
Status: Done
Archived in project
4 participants