Skip to content
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

Issue 195 #196

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Issue 195 #196

wants to merge 9 commits into from

Conversation

bckohan
Copy link
Contributor

@bckohan bckohan commented Jun 13, 2020

This PR is to merge in modifications to support the functionality described in issue 195. All changes are 100% backwards compatible. I've added two new test cases to validate the resource inclusion and alternate extensions. All tests pass on python 3.6-3.8 and there are no mypy or flake8 linting flags.

The basic strategy was to create a _Resource class that extends from str like _Option. This class serves as a thin wrapper over the new importlib_resources library calls. The path of the resource is set as the string. The one thing to note is that if the package containing the resources being imported is archived then importlib_resources will create a temporary file to read from. This file is cleaned up at the end of the include call.

The idea was to have resource files run through basically the same execution path as normal files to reduce the chance of bugs. The normal file execution code path is minimally touched to reduce the chances of any non-backwards compatibilities.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your time and effort!

Let me ask several questions about this feature, because not everything is clear to me 🙂
First, am I correct that the main motivation for this change is that IDE handles split_settings incorrectly?
Secondly, looks like .conf is really a python file but with an alternate extension, isn't it? Can it work with other file formats like .cfg or .ini or .env?

Cheers!

@bckohan
Copy link
Contributor Author

bckohan commented Jun 13, 2020

Hi Nikita,

First, am I correct that the main motivation for this change is that IDE handles split_settings incorrectly?

Not exactly, thats just a potential nice side effect. There are really two separate additions in this PR. The first is to be able to easily import setting snippets that are packaged with Django apps and installed from a repo like pypi. For instance, say you have an app that might have several different deployment profiles with somewhat complicated settings that define them, but you'd like to make it easy for your users to set it up for those different profiles. You're app structure might look like this:

myapp/
├── __init__.py
├── admin.py
├── apps.py
├── migrations
│   └── __init__.py
├── models.py
├── settings
│   ├── __init__.py
│   ├── profile1.py
│   ├── profile2.py
│   └── profile3.py
├── tests.py
└── views.py

Currently as a user of this example app and django-split-settings, if I installed the app from pypi and wanted to activate profile2 in my settings I could do this:

import importlib_resources
from split_settings import include

profile2 = importlib_resources.files('myapp.settings') / 'profile2.py'
with importlib_resources.as_file(profile2) as profile2_path:
    include(str(profile2_path))

This PR makes it so you could simply do:

from split_settings import include, resource

include(resource('myapp.settings', 'profile2.py'))

Secondly, looks like .conf is really a python file but with an alternate extension, isn't it? Can it work with other file formats like .cfg or .ini or .env?

Yes, .conf is just a python file. The second addition in this PR is allowing arbitrary file extensions. For instance, .settings or .django_settings. But this PR doesn't attempt to translate from other formats. There are several reasons I use different file extensions for the settings files in large code bases:

  • Given the size of the code bases >2000 files, ~100 Django apps, its nice to have an obvious visual cue to pick out your settings files. Calling them .conf or .settings achieves this nicely
  • My settings snippets often need to be included after other settings are defined, so many Python IDEs will call out erroneous variable undefined type errors that don't actually occur when the files are included as designed - usually based on the order of the INSTALLED_APPS stack.
  • You can package your settings files so they can't be imported normally. So other devs aren't tempted to do something like from myapp.settings.profile2 import *

For instance in the example above the way I do this is usually something like:

myapp/
├── __init__.py
├── admin.py
├── apps.py
├── migrations
│   └── __init__.py
├── models.py
├── settings
│   ├── profile1.conf
│   ├── profile2.conf
│   └── profile3.conf
├── tests.py
└── views.py

It's currently not possible to include these conf files with split_settings because the loader used only recognizes .py. This PR would allow an arbitrary file extension.

I realize this is a chunky PR, so if you'd rather consider the two additions separately, I'd be happy to separate them.

Brian

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants