search

mitodl / edx-sga

Staff Graded Assignment XBlock for the edX platform
GNU Affero General Public License v3.0
34 stars 109 forks source link

[BD-13][BB-6927] Rename descriptor to block #334

Closed pkulkark closed 1 year ago

pkulkark commented 1 year ago

Description

descriptor was renamed to block in https://github.com/openedx/edx-platform/pull/31492. This PR updates the corresponding imports.

Testing Instructions

This must be tested along with https://github.com/openedx/edx-platform/pull/31492.

pdpinch commented 1 year ago

@pkulkark is this ready for review?

codecov-commenter commented 1 year ago

Codecov Report

Merging #334 (8d2df05) into master (cb1a5f9) will decrease coverage by 0.04%. The diff coverage is 0.00%.

:exclamation: Current head 8d2df05 differs from pull request most recent head 7252050. Consider uploading reports for the commit 7252050 to get more accurate results

@@            Coverage Diff             @@
##           master     #334      +/-   ##
==========================================
- Coverage   53.35%   53.32%   -0.04%     
==========================================
  Files          16       16              
  Lines        1653     1654       +1     
  Branches      109      109              
==========================================
  Hits          882      882              
- Misses        760      761       +1     
  Partials       11       11
Impacted Files Coverage Δ
edx_sga/tests/integration_tests.py 0.00% <0.00%> (ø)
Agrendalath commented 1 year ago

@pdpinch, yes. This one is just a tiny refactor, as it renames "descriptors" to "blocks" to make the naming consistent with edx-platform.

arslanashraf7 commented 1 year ago

@Agrendalath Thanks for creating this PR and apologies that it took us long to get to it.

The changes look good. However, to make sure the integrity of the things could you try to rebase this PR off of master?

Agrendalath commented 1 year ago

@arslanashraf7, sure, done. The CI will probably fail, as I fixed it in https://github.com/mitodl/edx-sga/pull/339/commits/e111a7f401f37fe5d2f9476be124e2f87787bbd7.

arslanashraf7 commented 1 year ago

@arslanashraf7, sure, done. The CI will probably fail, as I fixed it in e111a7f.

@Agrendalath Thanks for doing that in a separate commit, But i think there would be more to be done to remove the codecov and I think we should do that in a separate PR. Let me create one for that and I might have to ask you to rebase again.

Agrendalath commented 1 year ago

@arslanashraf7,

But i think there would be more to be done to remove the codecov and I think we should do that in a separate PR.

All steps are already done in this PR: https://github.com/mitodl/edx-sga/pull/339. You can see that the codecov is working correctly there.

arslanashraf7 commented 1 year ago

@arslanashraf7,

But i think there would be more to be done to remove the codecov and I think we should do that in a separate PR.

All steps are already done in this PR: #339. You can see that the codecov is working correctly there.

I agree, It's just that there were some other places that would need changes e.g. this script used to run in travis but not anymore, But can still be run locally for tests. Plus an independent PR seems like a better way to keep the concerns separate. I've created https://github.com/mitodl/edx-sga/pull/340.

arslanashraf7 commented 1 year ago

@Agrendalath We merged the codecov fixes, Could you just do one last rebase on this PR? Ideally, the CI/tests should pass on this PR now.

Agrendalath commented 1 year ago

@arslanashraf7, done. Would you be able to review https://github.com/mitodl/edx-sga/pull/339, too? It has a much higher priority than this minor change.

arslanashraf7 commented 1 year ago

@arslanashraf7, done. Would you be able to review #339, too? It has a much higher priority than this minor change.

Thanks, I just re-ran the test on this. Yes, I'll try to review #339 today after this. Would you want to rebase that as well once you merge this?

Agrendalath commented 1 year ago

@arslanashraf7, is there anything else needed from our end before we merge?

Never mind, the "waiting on author" label confused me.

arslanashraf7 commented 1 year ago

@arslanashraf7, is there anything else needed from our end before we merge?

I just merged this. Could you do a final rebase on https://github.com/mitodl/edx-sga/pull/339?

arslanashraf7 commented 1 year ago

@arslanashraf7, is there anything else needed from our end before we merge?

Never mind, the "waiting on author" label confused me.

Yeah, That's just a general process.

Agrendalath commented 1 year ago

@arslanashraf7, sure, rebased.