-
Notifications
You must be signed in to change notification settings - Fork 224
Adds xblock-utils repository code into this repository #669
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
Conversation
d4776c4 to
ec610f2
Compare
3763d1f to
c616622
Compare
bda7660 to
2611c6e
Compare
feanil
left a comment
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 rather than bringing the xbolckutils module directly, I think we want to move all the relevant code into a xblock.utils module. This keeps the code in the xblocks repository cleaner in the long run but will involve a bit more complexity to the move. Can you try to update this so that the code lives under the existing xblocks structure?
2611c6e to
a5e51c9
Compare
@feanil Yes we can I kept it separate for following 2 pros:
But if we want to keep it intact with the xblocks repo, it should reside inside Let me refactor it. 1 more thing I am not going to add the bok-choy integration tests as we have deprecated the package |
That makes sense to me. |
87b946e to
76eaea3
Compare
4a49256 to
617d313
Compare
617d313 to
962adf4
Compare
feanil
left a comment
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 looks pretty good, there are a few more things I need to check before I can finish the review but so far this looks good. Can you link this PR to the corresponding PR in the xblock-utils repo?
8b1be5b to
4c47904
Compare
6c61401 to
279f341
Compare
feanil
left a comment
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.
A couple more comments and then I think this will be ready for review by Dave/Kyle.
feanil
left a comment
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 Just one more note about the version.
ormsbee
left a comment
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.
I did not review all the files in the utils and test/utils packages because I assume they're coming straight over from xblockutils (except for the bok choy stuff that was dropped).
I'm also assuming that some manual testing is being done to make sure that things still work when we have a mix of XBlocks where some XBlocks are using xblock.utils and some are using xblockutils and we're installing both libraries at the same time–since this mode is likely to be very common for a while.
Otherwise, it looks good to me.
eecd419 to
fdc1e17
Compare
|
Thanks for review and sharing thoughts @ormsbee
Yes they are coming straight from repo, no need to review all
Yes, some manual testing is done on DoneXBlock and repo is working fine with different pip package cases:
|
feanil
left a comment
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 I think you need to add a changelog entry to summarize that this is a move of code from the xblock-utils repository and then I think this is good to merge and release.
2f1a9fb to
f502eae
Compare
@feanil I have added the changelog and merged the docs PR into this PR as well. |
feanil
left a comment
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.
One small suggestion to the CHANGELOG and then I think this is good to merge and release.
d0f0da7 to
21cd9a4
Compare
21cd9a4 to
d6b7b2d
Compare
feanil
left a comment
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.
👍🏾 ![]()



Adds xblock-utils repository code into this repository
Relevant ticket: #675
Implementation details:
I have added
xblock-utilscode in thexblock/utilspackage and added their test cases inxblock/test/utilspackageI didn't move the bok-choy integration test cases into the repo because the library has been deprecated.
Child PR to move docs: #672