djangocms-installer icon indicating copy to clipboard operation
djangocms-installer copied to clipboard

Unneeded DATA_DIR kludge

Open greyhare opened this issue 5 years ago • 8 comments

I got my project generated, and I'm looking at the diffs between it and a Django-CMS 3.5 project I set up a year ago, and there's this weirdness at the start of settings.py:

import os  # isort:skip
gettext = lambda s: s
DATA_DIR = os.path.dirname(os.path.dirname(__file__))
"""
Django settings for abcde project.

Generated by 'django-admin startproject' using Django 2.1.9.

For more information on this file, see
https://docs.djangoproject.com/en/2.1/topics/settings/

For the full list of settings and their values, see
https://docs.djangoproject.com/en/2.1/ref/settings/
"""

import os

# Build paths inside the project like this: os.path.join(BASE_DIR, ...)
BASE_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))

There's a few problems here:

1. You can't put code in front of the file's docstring. Python will ignore it; it is not placed in the module's __doc__ property:

$ ./manage.py shell
>>> import abcde.settings as tgt_settings
>>> repr(tgt_settings.__doc__)
'None'

Worse, while Python itself ignores those strings, other tools such as epydoc consider strings after variable assignments to be docstrings for those variables (e.g. for documenting module constants).

2. DATA_DIR has the same value as BASE_DIR, and BASE_DIR's algorithm is more robust. DATA_DIR should be set equal to BASE_DIR.

3. Setting gettext to a no-op without documenting why, when we should be trying to import the proper gettext function, and only setting the no-op version if the import fails or something.

4. The os module is imported twice.

Corrected version:

"""
Django settings for abcde project.

Generated by 'django-admin startproject' using Django 2.1.9.

For more information on this file, see
https://docs.djangoproject.com/en/2.1/topics/settings/

For the full list of settings and their values, see
https://docs.djangoproject.com/en/2.1/ref/settings/
"""

import os
try:
    from gettext import gettext
except ImportError:
    def gettext(s):
        return s

# Build paths inside the project like this: os.path.join(BASE_DIR, ...)
BASE_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
DATA_DIR = os.environ.get('DATA_DIR', BASE_DIR)

greyhare avatar Jun 30 '19 23:06 greyhare

Unfortunately settings.py is generated by brute force automatic search / replace and must take into account a lot of different django versions. I have some plan to cleanup the code a bit when Django 2.2 / django CMS 3.7 will be available and we will be able to rely on a more reduced set of django / django CMS versions.

Any suggestion of PoC on how to improve the current code is obviously welcome!

yakky avatar Jul 02 '19 12:07 yakky

Django 2.2 has been out for about three months now. It's already up to 2.2.2. Django CMS... is taking its time, apparently. As for reparsing the settings file, it's not a trivial problem. Personally, I lean towards the cookicutter-django route.

For the DCMS project I'm working on now, I'm reorganizing it to look more like the cookiecutter layout.

greyhare avatar Jul 04 '19 03:07 greyhare

Unfortunately we must follow the Django CMS releases when it comes to Django support Cookiecutter is a really great project, and it's definitely a great approach. This project is less opinionated than cookiecutter due to its goal of providing an easy way to bootstrap a Django CMS project, without imposing it's own layout

yakky avatar Jul 04 '19 20:07 yakky

I wasn't suggesting using cookicutter-django itself, merely the cookiecutter tech it's built on.

It would be a significant change, though.

greyhare avatar Jul 04 '19 20:07 greyhare

Is DATA_DIR safe to delete and use BASE_DIR only?

Also, what's the use of the gettext function? Is it supposed to be replaced by a translation function or something?

eyadmba avatar Jan 02 '20 13:01 eyadmba

@bluegrounds gettext can be probably removed now as it should be safe to use the "real" gettext_lazy in settings. this used to be impossibile in settings.py and it remained in the generated settings file. We are testing a more intelligent way to patch settings.py via ast module but it will take some time for it to be mergeable (a few months). In the meantime we don't plan any cleanup on settings file

yakky avatar Jan 02 '20 16:01 yakky

Sounds awesome.

What about DATA_DIR though? Is it imported anywhere else outside settings.py?

eyadmba avatar Jan 04 '20 12:01 eyadmba

DATA_DIR is used to set MEDIA_ROOT, STATIC_ROOT

we will revise this upon the refactoring of the settings generation

yakky avatar Jan 04 '20 13:01 yakky