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

chore: fix depr warning by updating import: xblockutils -> xblock.utils #127

Closed
wants to merge 1 commit into from

Conversation

kdmccormick
Copy link
Contributor

@kdmccormick kdmccormick commented Jan 9, 2024

Context: openedx/XBlock#675

Fixes some edx-platform warnings:

2024-01-09 14:45:42,875 WARNING 289 [py.warnings] [user None] [ip None] warnings.py:109 - /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. 
  from xblockutils.publish_event import PublishEventMixin

2024-01-09 14:45:42,875 WARNING 289 [py.warnings] [user None] [ip None] warnings.py:109 - /openedx/venv/lib/python3.8/site-packages/poll/poll.py:41: DeprecatedPackageWarning: Please use import xblock.utils.resources instead of xblockutils.resources because the 'xblock-utils' package has been deprecated and migrated to within 'xblock' package. 
  from xblockutils.resources import ResourceLoader

2024-01-09 14:45:42,875 WARNING 289 [py.warnings] [user None] [ip None] warnings.py:109 - /openedx/venv/lib/python3.8/site-packages/poll/poll.py:42: DeprecatedPackageWarning: Please use import xblock.utils.settings instead of xblockutils.settings because the 'xblock-utils' package has been deprecated and migrated to within 'xblock' package. 
  from xblockutils.settings import ThemableXBlockMixin, XBlockWithSettingsMixin

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

Bumps version from 1.13.0 to 1.13.1

Resolves #114.

Copy link
Member

@Agrendalath Agrendalath left a comment

Choose a reason for hiding this comment

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

Thanks for preparing this, @kdmccormick!

One request - could we make this change backward-compatible, as, e.g., we did here (ref)?

We won't need these fallbacks in tests, though.

@kdmccormick kdmccormick force-pushed the master branch 2 times, most recently from 80ab62d to a6eb0c6 Compare January 9, 2024 15:55
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
Copy link
Contributor Author

Sure thing @Agrendalath . Ready for another look.

@Agrendalath
Copy link
Member

@kdmccormick:

  1. It seems that the Selenium test mixins were not moved to the XBlock package. Should we remove Selenium tests, as we did in other repos?
  2. If we decide to go with the approach from the previous point, we should replace xblock-utils with XBlock[django] in requirements/base.in.

@kdmccormick
Copy link
Contributor Author

Good questions @Agrendalath . I wasn't aware that not all modules were copied from xblockutils to xblock.utils.

I see the justification for removing base_test.py is that bok-choy is deprecated, but it seems like xblock-poll uses base_test.py for integration testing without actually relying on bok-choy at all. The integration tests seem to be working fine in this repository and I'd hate to remove them for no reason. I'm wondering now whether it was a mistake to drop base_test.py. What do you think?

@bradenmacdonald
Copy link
Member

It looks like base_test.py is still useful yep, as it can be used for "plain" selenium integration tests, without any bok-choy. In fact there's no code related to bok-choy in that file that I can see.

My suggestion is that since base_test.py is so tightly coupled with workbench, we move it into the xblock-sdk repo and combine it with test_utils.py which seems to have related functionality.

@farhan
Copy link

farhan commented Feb 7, 2024

@kdmccormick its under discussion here
openedx/xblock-sdk#345 (comment)

@kdmccormick
Copy link
Contributor Author

@bradenmacdonald and @Agrendalath : @farhan dug in and found that SeleniumXBlockTest was actually using bok-choy's WebAppTest as a base, so it seems that it was right to remove it instead of pulling it into XBlock.

Unless you have any objections, I'll remove this XBlock's selenium tests and update requirements/base.in to point at XBlock[django].

@kdmccormick
Copy link
Contributor Author

This ended up being more work than I'm able to take on right now. I'm going to add some notes to #114 and close this. Feel free to re-use this PR in the future if that's helpful.

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.

Remove xblock-utils dependency
4 participants