-
-
Notifications
You must be signed in to change notification settings - Fork 71
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: Prepare autodiscover and template loader for v1 #533
refactor: Prepare autodiscover and template loader for v1 #533
Conversation
matched_files: List[Path] | ||
|
||
|
||
def search(search_glob: Optional[str] = None, engine: Optional[Engine] = None) -> SearchResult: |
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've split this function to get_dirs
and search_dirs
and moved them to autodiscover.py
if hasattr(settings, "BASE_DIR"): | ||
path = (Path(settings.BASE_DIR) / component_dir).resolve() | ||
path: Path = (Path(settings.BASE_DIR) / component_dir).resolve() |
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.
This branch allows us to treat a relative path in STATICFILES_DIRS
as relative to the project root. The question is whether we want to support this, since in docs we ask users to set the dirs as:
STATICFILES_DIRS = [
...,
os.path.join(BASE_DIR, "components"),
]
At first I was in favour as it could be nicer experience. However, looking at Django's docs on STATICFILES_DIRS, they ask for full paths. So if we on the other hand ask users to put relative paths into STATICFILES_DIRS
, it could lead to unexpected behaviour. Instead, we should expect STATICFILES_DIRS
to have full paths only.
Which means we'd remove the feature of supplying relative paths to STATICFILES_DIRS
. I think this is fine, as resolving the paths relative to BASE_DIR
is simple to be done on user-side:
Path(settings.BASE_DIR) / component_dir).resolve()
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.
UPDATE: I've refactored the Loader to expect only full paths from STATICFILES_DIRS
.
if path.is_dir(): | ||
curr_directories.add(path) | ||
|
||
# Add the directory that holds the settings file | ||
# NOTE: This applies only to relative paths | ||
if settings.SETTINGS_MODULE: |
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.
Same issue is here as I mention in https://github.com/EmilStenstrom/django-components/pull/533/files#r1649603172. Basically we treat the dirs in STATICFILES_DIRS
as relative altho Django docs ask for absolute paths.
At first I didn't understand why we resolve relative to SETTINGS_MODULE
, but turns out that was the initial implementation in https://github.com/EmilStenstrom/django-components/blob/58c6e476f1da7a227512f360946fc8f4f88796eb/django_components/template_loader.py. And then, couple of months ago from now, it was branched out in https://github.com/EmilStenstrom/django-components/blob/567c8ccc3d5747bc01a0a4b83c6d54f3880c4c7a/django_components/template_loader.py to fix #290.
Correct me if wrong, but I think the code branch can be summarized as "Look for component_dir
as relative subpath in settings dir (e.g. my_app/settings/
), the app where settings are set (e.g. my_app
), and the root".
If that's right, we're practically trying to get to the same place as we do in the code branch above with settings.BASE_DIR
. So we could just use BASE_DIR
instead of relying on SETTINGS_MODULE
.
And as I mention in the comment above on BASE_DIR
, I think that it's simple enough to use to let users specify the full path via BASE_DIR
as:
Path(settings.BASE_DIR) / component_dir).resolve()
So personally I would remove this code block for settings.SETTINGS_MODULE
, and tell users to replace that with something like:
Path(settings.BASE_DIR) / "app_with_settings" / "settings").resolve()
Is there maybe something I'm missing about settings.SETTINGS_MODULE
?
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.
UPDATE: I've refactored the Loader to use BASE_DIR
instead of SETTINGS_MODULE
# but the default suggests relative, and code suggests both. Document all the supported cases. | ||
# -> Why do we add settings directory? | ||
# -> Could we merge this with `autodiscover_modules()` in `autodiscover.py`, since there we also | ||
# look for `[app_name]/components`, or do these two accomplish different things? |
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.
Q&A:
-
Are STATICFILES_DIRS absolute paths or relative to project root? README suggest absolute, but the default suggests relative, and code suggests both. Document all the supported cases.
- I've already touched on this in other comments . But Django asks for full paths.
-
Why do we add settings directory?
-
Could we merge this with
autodiscover_modules()
inautodiscover.py
, since there we also look for[app_name]/components
, or do these two accomplish different things?- No.
autodiscover_modules
imports individual modules (AKA files), whereas inSTATICFILES_DIRS
, we set DIRS as inputs.
- No.
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.
TODO: Use this for updating README
src/django_components/apps.py
Outdated
# | ||
# TODO - Allow only if autodiscovery is true? | ||
# Because right now it's kinda weird that we import the COMPONENTS.libraries | ||
# Even if autodiscovery is off. | ||
self.module.autodiscover() |
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 think it makes sense to make the autodiscover
function as part of the public API, so that people can possibly run it when they need to. However, that invites a discussion on how autodiscover
should behave:
So autodiscover currently actually imports from 3 places:
- Component modules -
[app_name]/components.py
for each app in the project - Component dirs - As defined by
Loader.get_dirs
, which takes theSTATICFILES_DIRS
, and for each one tries to look in a couple of places - Libraries - Modules listed in
COMPONENTS.libraries
.
How things worked up to now is that:
-
- and 2., so
[app_name]/components.py
andSTATICFILES_DIRS
, were loaded only ifAUTODISCOVER
setting was ON.
- and 2., so
- 3., the
COMPONENTS.libraries
, were ALWAYS imported.
This feels a bit random, and raises a few questions:
-
Should we split the logic or rename the function? The "autodiscovery" is really only the part where we search
STATICFILES_DIRS
for python files. The importing ofCOMPONENTS.libraries
and[app_name]/components.py
is more like "import_libraries" or "import_modules" -
Should we drop support for automatically searching for
[app_name]/components.py
, and let users set that manually viaCOMPONENTS.libraries
? Or we can externalize it as a helper function that generates a list of paths like[app_name]/components.py
, so users can then pass the results toCOMPONENTS.libraries
.Note that the
[app_name]/components.py
feature is NOT documented currently.from django_components.utils import gen_app_paths COMPONENTS = { "libraries": [ # Generates `[app_name]/components.py` for each app *gen_app_paths("components"), # Other "my_app.some.path", ... ], }
-
How should we handle when
AUTODISCOVER
is ON / OFF?
First of all, I think we should remove[app_name]/components.py
(1. from the beginning of this comment) and let users set the same thing viaCOMPONENTS.libraries
Then, there would be only imports via
COMPONENTS.libraries
(3.) andSTATICFILES_DIRS
(2.).- If user wanted to turn off imports via
COMPONENTS.libraries
, they could just set it to an empty list - And if user wanted to turn off imports from
STATICFILES_DIRS
, they would setAUTODISCOVER
toFalse
.
So that means that
AUTODISCOVER
would only decide whether to search for python files inSTATICFILES_DIRS
. - If user wanted to turn off imports via
So all-in-all, my approach would be:
-
The
autodiscover.py
file would export two functions:autodiscover
andimport_libraries
-
autodiscover
function would have signaturedef autodiscover( map_module_names: Optional[Callable[[str], str]] = None, ) -> List[str]:
Where:
map_module_names
is renamed frommap_components_modules
, and it is kept as we use that in some of the tests.autodiscover
function returns a list of module names that were importedautodiscover
function only searchesSTATICFILES_DIRS
for python files, and does NOT import fromCOMPONENTS.libraries
, nor from[app_name]/components.py
.autodiscover
function would run inside theready
hook only ifAUTODISCOVER
setting is ON. However, user could import the function and run it themselves whenever.
-
import_libraries
function would have signaturedef import_libraries( map_module_names: Optional[Callable[[str], str]] = None, ) -> List[str]:
Where:
map_module_names
is added to have the same functionality asautodiscover
import_libraries
function returns a list of module names that were importedimport_libraries
only imports fromCOMPONENTS.libraries
, nothing else.import_libraries
would run inside theready
hook ALWAYS. However, user could import the function and run it themselves whenever.
So src/django_components/apps.py would look like this:
from django.apps import AppConfig
class ComponentsConfig(AppConfig):
name = "django_components"
def ready(self) -> None:
# This is the code that gets run when user adds django_components to Django's INSTALLED_APPS
from django_components.app_settings import app_settings
from django_components.autodiscover import autodiscover, import_libraries
import_libraries()
if app_settings.AUTODISCOVER:
autodiscover()
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.
UPDATE: Updated as described above
@EmilStenstrom This is a WIP, but I'm eager to hear your thoughts on this, especially on #533 (comment) |
eb13c70
to
e156263
Compare
for more information, see https://pre-commit.ci
… and make test setup configurable
for more information, see https://pre-commit.ci
|
||
if django.VERSION < (3, 2): | ||
default_app_config = "django_components.apps.ComponentsConfig" | ||
|
||
|
||
def autodiscover(map_import_paths: Optional[Callable[[str], str]] = None) -> List[str]: |
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.
Moved autodiscovery to its own file
component_dirs = settings.STATICFILES_DIRS | ||
else: | ||
component_dirs = ["components"] | ||
component_dirs = [settings.BASE_DIR / "components"] |
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.
This has to be now full path instead of relative path, because also settings.STATICFILES_DIRS
must be absolute paths.
# and, for each app, check if the STATICFILES_DIRS dir is within that app dir. | ||
# If so, we add the dir as a valid source. | ||
# The for loop is based on Django's `get_app_template_dirs`. | ||
for app_config in apps.get_app_configs(): |
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.
Removed the IF logic on this line (https://github.com/EmilStenstrom/django-components/pull/533/files#diff-756de4c35ec676b6ad61e551979238b310a7806c7c612add84aaec6f376c1b16L65):
if hasattr(settings, "BASE_DIR"):
So that we treat STATICFILES_DIRS as full paths. This means that if people previously specified relative paths in STATICFILES_DIRS, they will now have to set them to full paths themselves.
Also removed the IF logic on this line https://github.com/EmilStenstrom/django-components/pull/533/files#diff-756de4c35ec676b6ad61e551979238b310a7806c7c612add84aaec6f376c1b16L71
if settings.SETTINGS_MODULE:
Instead of relying on SETTINGS_MODULE
, we now use only STATICFILES_DIRS and BASE_DIR. So if people used SETTINGS_MODULE
before, they will have to set BASE_DIR
instead.
import django | ||
from django.conf import settings | ||
|
||
if not settings.configured: | ||
|
||
def setup_test_config(components: Optional[Dict] = None): |
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.
Instead of configuring Django setting when we import django_test_setup
, it's now wrapped in a function, so we can configure the settings.
@@ -0,0 +1,76 @@ | |||
from pathlib import Path |
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've split the test_autodiscover.py
file and move the tests for template loader into test_template_loader.py
.
@@ -89,7 +89,7 @@ def autodiscover_with_cleanup(*args, **kwargs): | |||
ContextBehParam = Union[ContextBehStr, Tuple[ContextBehStr, Any]] | |||
|
|||
|
|||
def parametrize_context_behavior(cases: List[ContextBehParam]): | |||
def parametrize_context_behavior(cases: List[ContextBehParam], settings: Optional[Dict] = None): |
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.
Updated this test helper so that we can pass also other settings to override_settings
.
As of now, I still need to:
|
Happy to report that everything works well both in sampleproject, and in my own Django project! So this MR is now ready for review @EmilStenstrom |
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.
Excellent additions and clearups as usual. I've added a couple of comments and thoughts, but all in all looks good.
Thanks again for all the feedback! |
MR for moving autodiscover to it's own file + documenting it, as mentioned in #527 (comment).