-
Notifications
You must be signed in to change notification settings - Fork 70
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
refactor: Remove bok-choy and xblockutils (use xblock.utils instead) packages #357
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@farhan, this should define a minimum version for the XBlock
dependency in the requirements/base.in
.
We should bump the major version here if we do not provide the fallback import from xblockutils
. We should also clearly state which of the edx-platform named releases are compatible with this new version.
e0f9fc4
to
07dee3e
Compare
@farhan, why are we removing integration tests? Are we going to replace them with something else? |
@Agrendalath We are removing bok-choy integration tests because the library has been deprecated. Bok-choy integration test has not been copied from xblock-utils to xblock/tests/utils due to which the repositories that are using the base test classes of xblock/integration/tests are breaking. We are already planning to remove bok-choy test cases, so covering it in this pr as well. |
70c5189
to
702ca32
Compare
@farhan few test cases are failing like |
0edffcc
to
571d5dd
Compare
@Agrendalath
I have added the details in the change log, should we state this change on some other place as well? |
What do you mean by "all of it"?
This is a breaking change, so we should indicate this in the commit, PR's name + description, and the changelog. Please take a look at #318 for an example of how we have been doing it. Also, I wonder if |
@Agrendalath I want to ask please verify all the removal done in this PR seems right to you
That's true there is no breaking change in xblock release v1.8.0 and v1.8.1 |
571d5dd
to
7d7a378
Compare
@Agrendalath |
98a5971
to
40f6b3e
Compare
* Removed xblock-utils package: Replace xblockutils.* imports with xblock.utils.* imports * Removed bok-choy package and its relevant test cases
40f6b3e
to
cba7dca
Compare
224eee0
to
9754a3e
Compare
@Agrendalath Addressed all the requested changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@farhan, thanks for working on this!
👍
- I tested this: checked that the XBlock is working correctly with Nutmeg and
master
versions of the devstack - I read through the code
- I checked for accessibility issues: n/a
- Includes documentation: n/a
Description
Breaking change in tests
There is no breaking change in the xblock feature in the implementation of this PR
As we have parted ways with the bok-choy library this PR is removing all the integrating test cases along with the base test classes because they are linked with the library.
Notes
Resolves #356
Resolves #354