Skip to content
This repository has been archived by the owner on Oct 5, 2023. It is now read-only.

Deprecate the repository and pip package #231

Merged
merged 7 commits into from
Oct 4, 2023
Merged

Conversation

farhan
Copy link
Contributor

@farhan farhan commented Sep 26, 2023

Ticket: openedx/XBlock#675

PR implementation details:
Add a deprecation warning in:

  1. a readme file
  2. all the modules to show a warning while importing deprecated modules from a deprecated package

For readme preview:
https://github.com/openedx/xblock-utils/blob/farhan/deprecate-repo/README.rst

Implementation inspiration:
https://github.com/openedx/edx-platform/blob/master/docs/decisions/0007-sys-path-modification-removal.rst
https://github.com/openedx/edx-platform/tree/open-release/koa.1/import_shims

How deprecated warning will appear in the console:

Screenshot 2023-09-27 at 12 14 49 PM

@salman2013
Copy link
Contributor

@farhan Could you please look at the test failure.

@farhan farhan requested a review from salman2013 September 26, 2023 15:19
@farhan
Copy link
Contributor Author

farhan commented Sep 26, 2023

@farhan Could you please look at the test failure.

@salman2013 fixed

@farhan farhan changed the title mention repo deprecation in the readme Add deprecation warnings Sep 27, 2023
@farhan farhan force-pushed the farhan/deprecate-repo branch 2 times, most recently from bc4c0c9 to 5c239df Compare September 27, 2023 11:19
@farhan farhan requested a review from feanil September 27, 2023 12:09
Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

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

@farhan we should also remove the old code from these files and replace it with wildcard imports of the new locations. That way we are not maintaining two copies of the code that can get out of sync and the shell of xblock-utils continues to work as we make changes in the upstream repo.

@farhan farhan force-pushed the farhan/deprecate-repo branch from 5c239df to 497dfe8 Compare October 2, 2023 05:02
Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

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

Looks like this is on the right track and should be good to land once the tests have been fixed and you've tested locally that an XBlock that still uses xblock-utils runs and produces the expected warnings.

@farhan
Copy link
Contributor Author

farhan commented Oct 3, 2023

Looks like this is on the right track and should be good to land once the tests have been fixed and you've tested locally that an XBlock that still uses xblock-utils runs and produces the expected warnings.

@feanil test cases fixed
I have done the testing on the DoneXblock
I installed the xblock-utils zip 4.0.0 with the command pip install xblock-utils-4.0.0.zip
and verify the working of the Done Xblock

Terminal Screen Shots
Screenshot 2023-10-03 at 3 20 52 PM
Screenshot 2023-10-03 at 3 20 27 PM

🤔
Shouldn't we remove the test case in this repository as these test cases are running on the xblock.utils package while we are already running the test cases within xblock repo as well?

@feanil
Copy link
Contributor

feanil commented Oct 3, 2023

@farhan yea, once we're happy with this, we can drop all the tests but it's useful to have them to make sure we did all the import redirecting correctly. It makes sense that those tests would need to be updated because of how the ResourceLoader class is being imported now within the XBlock library but it was good to verify we didn't break any non-test functionality of calling the imports the old way in the tests.

Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

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

Changes look great, I left a minor suggestion in the language of the import warning that you can take or leave. Feel free to merge and make a final release of this repo.

xblockutils/deprecation/warn.py Outdated Show resolved Hide resolved
@farhan farhan changed the title Add deprecation warnings Deprecate the repository and pip package Oct 4, 2023
@farhan farhan merged commit 7a88cb9 into master Oct 4, 2023
4 checks passed
@farhan farhan deleted the farhan/deprecate-repo branch October 4, 2023 04:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants