Rename sys_path_hacks/ to import_shims/
The old folder name is somewhat confusing, because the folder contains shims to _compensate for the removal of sys.path hacks_, but does not contain the sys.path hacks themselves. Furthermore, this import_shims/ system could also be used for other import path changes, such as turning the locally-installed packages in common/lib/ into regular, importable modules (e.g. `from common.lib.xmodule import abc` instead of `from xmodule import abc`). So, a name that is not specific to the sys.path hacks may be better in the medium-to-long term. Along the same lines, we also rename SysPathHackWarning to DeprecatedEdxPlatformImportWarning.
This commit is contained in:
Kyle McCormick
committed by
Kyle McCormick
Kyle McCormick
parent
644963255c
commit
090e10683c
@@ -448,7 +448,7 @@ GITHUB_REPO_ROOT = ENV_ROOT / "data"
|
||||
sys.path.append(REPO_ROOT)
|
||||
# TODO: The next two path modifications will be removed in an upcoming Open edX release.
|
||||
# See docs/decisions/0007-sys-path-modification-removal.rst
|
||||
sys.path.append(REPO_ROOT / 'sys_path_hacks' / 'studio')
|
||||
sys.path.append(REPO_ROOT / 'import_shims' / 'studio')
|
||||
sys.path.append(COMMON_ROOT / 'djangoapps')
|
||||
|
||||
# For geolocation ip database
|
||||
|
||||
@@ -24,11 +24,11 @@ In order to simplify the edx-platform code environment, we will remove the modif
|
||||
|
||||
This deprecation will take place in the following steps:
|
||||
|
||||
1. Add a new folder (``sys_path_hacks``) to isolate the code used for deprecation warnings.
|
||||
1. Add a new folder (``import_shims``) to isolate the code used for deprecation warnings.
|
||||
|
||||
2. For every module importable using the ``sys.path`` style (for instance, ``courseware``), duplicate that module structure into the ``sys_path_hacks/lms`` (or ``sys_path_hacks/studio``) directory. Each file in that directory should do a wild-card import of the corresponding ``lms.djangoapps.`` module, and should log a warning indicating where it was imported from. For example, in ``sys_path_hacks/lms/courseware/views/views.py``, it will wild-card import ``from lms.djangoapps.courseware.views.views import *``. The ``sys_path_hacks/un_sys_path.sh`` script will generate these files.
|
||||
2. For every module importable using the ``sys.path`` style (for instance, ``courseware``), duplicate that module structure into the ``import_shims/lms`` (or ``import_shims/studio``) directory. Each file in that directory should do a wild-card import of the corresponding ``lms.djangoapps.`` module, and should log a warning indicating where it was imported from. For example, in ``import_shims/lms/courseware/views/views.py``, it will wild-card import ``from lms.djangoapps.courseware.views.views import *``. The ``import_shims/generate_shims.sh`` script will generate these files.
|
||||
|
||||
3. The ``sys.path`` modification will be changed to point to ``sys_path_hacks/lms``, rather than ``lms/djangoapps``. At this point, any code that references the modules directly will trigger warnings with logging about where the imports were coming from (to drive future cleanup efforts). The warnings will be instances of ``SysPathHackWarning`` (subclass of ``DeprecationWarning``).
|
||||
3. The ``sys.path`` modification will be changed to point to ``import_shims/lms``, rather than ``lms/djangoapps``. At this point, any code that references the modules directly will trigger warnings with logging about where the imports were coming from (to drive future cleanup efforts). The warnings will be instances of ``DeprecatedEdxPlatformImportWarning`` (subclass of ``DeprecationWarning``).
|
||||
|
||||
4. Fix all instances where the ``sys.path``-based modules were ``patch``-ed in unit tests, as those patches no longer work.
|
||||
|
||||
|
||||
57
import_shims/generate_shims.sh
Executable file
57
import_shims/generate_shims.sh
Executable file
@@ -0,0 +1,57 @@
|
||||
#!/usr/bin/env bash
|
||||
#
|
||||
# Usage:
|
||||
#
|
||||
# ~/edx-platform> import_shims/generate_shims.sh SOURCE DESTINATION
|
||||
#
|
||||
# where the modules in SOURCE should recursively have shims generated in DESTINATION,
|
||||
# which should be a subfolder of import_shims/.
|
||||
#
|
||||
# SOURCE and DESTINATION must both be relative to the root of edx-platform,
|
||||
# and must not include trailing slashes.
|
||||
#
|
||||
# For example:
|
||||
#
|
||||
# ~/edx-platform> import_shims/generate_shims.sh common/djangoapps import_shims/studio
|
||||
#
|
||||
# will mirror the packages structure of `common/djangoapps` within `import_shims/studio`.
|
||||
# One would run this if they want to mimic the effect of adding 'common/djangoapps'
|
||||
# to `sys.path` within Studio.
|
||||
|
||||
# Shellcheck recommends using search/replace instead of sed. It's fine as is.
|
||||
# shellcheck disable=SC2001
|
||||
|
||||
set -e
|
||||
set -o pipefail
|
||||
set -u
|
||||
|
||||
SOURCE="$1"
|
||||
PYTHON_SOURCE="${SOURCE/\//.}"
|
||||
DESTINATION="$2"
|
||||
for path in $(find "${SOURCE}/" -name '*.py' | grep -v migrations); do
|
||||
if [[ "$path" == "${SOURCE}/__init__.py" ]]; then
|
||||
# Skip unnecessary root __init__.py.
|
||||
continue
|
||||
fi
|
||||
if [[ "$path" == "lms/djangoapps/courseware/management/commands/import.py" ]]; then
|
||||
# Skip this file because its name is problematic for the sys path hack.
|
||||
# We've gone to prod with this excluded, and it hasn't been a problem.
|
||||
continue
|
||||
fi
|
||||
if [[ "$path" == "cms/djangoapps/contentstore/management/commands/import.py" ]]; then
|
||||
# Also skip this file because its name is problematic for the sys path hack.
|
||||
continue
|
||||
fi
|
||||
new_path=$(echo "$path" | sed "s#${SOURCE}/#${DESTINATION}/#")
|
||||
new_python_path=$(echo "$path" | sed "s#/#.#g" | sed "s#.py##" | sed "s#.__init__##")
|
||||
old_python_path=$(echo "$new_python_path" | sed "s#${PYTHON_SOURCE}.##")
|
||||
echo "Writing ${new_path}"
|
||||
mkdir -p "$(dirname "$new_path")"
|
||||
{
|
||||
echo "from import_shims.warn import warn_deprecated_import"
|
||||
echo
|
||||
echo "warn_deprecated_import('${old_python_path}', '${new_python_path}')"
|
||||
echo
|
||||
echo "from ${new_python_path} import *"
|
||||
} > "$new_path"
|
||||
done
|
||||
45
import_shims/warn.py
Normal file
45
import_shims/warn.py
Normal file
@@ -0,0 +1,45 @@
|
||||
"""
|
||||
Utilities for warning about deprecated imports temporarily supported by
|
||||
the import_shim/ system.
|
||||
|
||||
See /docs/decisions/0007-sys-path-modification-removal.rst for details.
|
||||
"""
|
||||
|
||||
import warnings
|
||||
|
||||
|
||||
class DeprecatedEdxPlatformImportWarning(DeprecationWarning):
|
||||
"""
|
||||
A warning that a module is being imported from an unsupported location.
|
||||
|
||||
Example use case:
|
||||
edx-platform modules should be imported from the root of the repository.
|
||||
|
||||
For example, `from lms.djangoapps.course_wiki import views` is good.
|
||||
|
||||
However, we historically modify `sys.path` to allow importing relative to
|
||||
certain subdirectories. For example, `from course_wiki ipmort views` currently
|
||||
works.
|
||||
|
||||
We want to stardize on the prefixed version for a few different reasons.
|
||||
"""
|
||||
|
||||
def __init__(self, old_import, new_import):
|
||||
super().__init__()
|
||||
self.old_import = old_import
|
||||
self.new_import = new_import
|
||||
|
||||
def __str__(self):
|
||||
return (
|
||||
"Importing {self.old_import} instead of {self.new_import} is deprecated"
|
||||
).format(self=self)
|
||||
|
||||
|
||||
def warn_deprecated_import(old_import, new_import):
|
||||
"""
|
||||
Warn that a module is being imported from its old location.
|
||||
"""
|
||||
warnings.warn(
|
||||
DeprecatedEdxPlatformImportWarning(old_import, new_import),
|
||||
stacklevel=3, # Should surface the line that is doing the importing.
|
||||
)
|
||||
@@ -706,7 +706,7 @@ DATA_DIR = COURSES_ROOT
|
||||
sys.path.append(REPO_ROOT)
|
||||
# TODO: The next two path modifications will be removed in an upcoming Open edX release.
|
||||
# See docs/decisions/0007-sys-path-modification-removal.rst
|
||||
sys.path.append(REPO_ROOT / 'sys_path_hacks' / 'lms')
|
||||
sys.path.append(REPO_ROOT / 'import_shims' / 'lms')
|
||||
sys.path.append(COMMON_ROOT / 'djangoapps')
|
||||
|
||||
# For Node.js
|
||||
|
||||
@@ -13,7 +13,7 @@ from path import Path
|
||||
# TODO: Remove the rest of the sys.path modification here and in (cms|lms)/envs/common.py
|
||||
REPO_ROOT = Path(__file__).abspath().dirname().dirname().dirname() # /edx-platform/
|
||||
sys.path.append(REPO_ROOT / 'common' / 'djangoapps')
|
||||
sys.path.append(REPO_ROOT / 'sys_path_hacks' / 'lms')
|
||||
sys.path.append(REPO_ROOT / 'import_shims' / 'lms')
|
||||
|
||||
ALL_LANGUAGES = []
|
||||
|
||||
|
||||
4
pylintrc
4
pylintrc
@@ -71,7 +71,7 @@
|
||||
ignore = ,.git,.tox,migrations,node_modules,.pycharm_helpers
|
||||
persistent = yes
|
||||
load-plugins = edx_lint.pylint,pylint_django,pylint_celery
|
||||
init-hook = "import sys; sys.path.extend(['sys_path_hacks/lms', 'sys_path_hacks/studio', 'common/djangoapps'])"
|
||||
init-hook = "import sys; sys.path.extend(['import_shims/lms', 'import_shims/studio', 'common/djangoapps'])"
|
||||
|
||||
[MESSAGES CONTROL]
|
||||
enable =
|
||||
@@ -464,4 +464,4 @@ int-import-graph =
|
||||
[EXCEPTIONS]
|
||||
overgeneral-exceptions = Exception
|
||||
|
||||
# 0981d49b0645874588dcf4423195239dd9e414b7
|
||||
# 1c68aee72f4071c31e5303be5610497ef5e64b7c
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
# pylintrc tweaks for use with edx_lint.
|
||||
[MASTER]
|
||||
ignore+ = ,.git,.tox,migrations,node_modules,.pycharm_helpers
|
||||
init-hook="import sys; sys.path.extend(['sys_path_hacks/lms', 'sys_path_hacks/studio', 'common/djangoapps'])"
|
||||
init-hook="import sys; sys.path.extend(['import_shims/lms', 'import_shims/studio', 'common/djangoapps'])"
|
||||
|
||||
[MESSAGES CONTROL]
|
||||
disable+ =
|
||||
|
||||
@@ -1,54 +0,0 @@
|
||||
#!/usr/bin/env bash
|
||||
#
|
||||
# Usage:
|
||||
#
|
||||
# sys_path_hacks/un_sys_path.sh SOURCE DESTINATION
|
||||
#
|
||||
# where SOURCE is the folder from which modules will be recursively copied
|
||||
# and DESTINATION is the subfolder of `sys_path_hacks` in which they end up.
|
||||
#
|
||||
# For example:
|
||||
#
|
||||
# ~/edx-platform> sys_path_hacks/un_sys_path.sh common/djangoapps studio
|
||||
#
|
||||
# will mirror the packages structure of `common/djangoapps` within `sys_path_hacks/studio`.
|
||||
# One would run this if they want to mimic the effect of adding 'common/djangoapps'
|
||||
# to `sys.path` within Studio.
|
||||
|
||||
# Shellcheck recommends using search/replace instead of sed. It's fine as is.
|
||||
# shellcheck disable=SC2001
|
||||
|
||||
set -e
|
||||
set -o pipefail
|
||||
set -u
|
||||
|
||||
SOURCE="$1"
|
||||
PYTHON_SOURCE="${SOURCE/\//.}"
|
||||
DESTINATION="$2"
|
||||
for path in $(find "${SOURCE}/" -name '*.py' | grep -v migrations); do
|
||||
if [[ "$path" == "${SOURCE}/__init__.py" ]]; then
|
||||
# Skip unnecessary root __init__.py.
|
||||
continue
|
||||
fi
|
||||
if [[ "$path" == "lms/djangoapps/courseware/management/commands/import.py" ]]; then
|
||||
# Skip this file because its name is problematic for the sys path hack.
|
||||
# We've gone to prod with this excluded, and it hasn't been a problem.
|
||||
continue
|
||||
fi
|
||||
if [[ "$path" == "cms/djangoapps/contentstore/management/commands/import.py" ]]; then
|
||||
# Also skip this file because its name is problematic for the sys path hack.
|
||||
continue
|
||||
fi
|
||||
new_path=$(echo "$path" | sed "s#${SOURCE}/#sys_path_hacks/${DESTINATION}/#")
|
||||
python_path=$(echo "$path" | sed "s#/#.#g" | sed "s#.py##" | sed "s#.__init__##")
|
||||
old_python_path=$(echo "$python_path" | sed "s#${PYTHON_SOURCE}.##")
|
||||
echo "Writing ${new_path}"
|
||||
mkdir -p "$(dirname "$new_path")"
|
||||
{
|
||||
echo "from sys_path_hacks.warn import warn_deprecated_import"
|
||||
echo
|
||||
echo "warn_deprecated_import('${PYTHON_SOURCE}', '${old_python_path}')"
|
||||
echo
|
||||
echo "from ${python_path} import *"
|
||||
} > "$new_path"
|
||||
done
|
||||
@@ -1,44 +0,0 @@
|
||||
"""
|
||||
Utilities for warning about deprecated imports supported by the sys_path_hack/ system.
|
||||
|
||||
See /docs/decisions/0007-sys-path-modification-removal.rst for details.
|
||||
"""
|
||||
|
||||
import warnings
|
||||
|
||||
|
||||
class SysPathHackWarning(DeprecationWarning):
|
||||
"""
|
||||
A warning that a module is being imported from its old, non-prefixed location.
|
||||
|
||||
edx-platform modules should be imported from the root of the repository.
|
||||
For example, `from lms.djangoapps.course_wiki import views` is good.
|
||||
|
||||
However, we historically modify `sys.path` to allow importing relative to
|
||||
certain subdirectories. For example, `from course_wiki ipmort views` currently
|
||||
works.
|
||||
|
||||
We want to stardize on the prefixed version for a few different reasons.
|
||||
"""
|
||||
|
||||
def __init__(self, import_prefix, unprefixed_import_path):
|
||||
super().__init__()
|
||||
self.import_prefix = import_prefix
|
||||
self.unprefixed_import_path = unprefixed_import_path
|
||||
self.desired_import_path = import_prefix + "." + unprefixed_import_path
|
||||
|
||||
def __str__(self):
|
||||
return (
|
||||
"Importing {self.unprefixed_import_path} instead of "
|
||||
"{self.desired_import_path} is deprecated"
|
||||
).format(self=self)
|
||||
|
||||
|
||||
def warn_deprecated_import(import_prefix, unprefixed_import_path):
|
||||
"""
|
||||
Warn that a module is being imported from its old, non-prefixed location.
|
||||
"""
|
||||
warnings.warn(
|
||||
SysPathHackWarning(import_prefix, unprefixed_import_path),
|
||||
stacklevel=3, # Should surface the line that is doing the importing.
|
||||
)
|
||||
Reference in New Issue
Block a user