nephila / djangocms-installer

Console wizard to bootstrap django CMS projects
https://djangocms-installer.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
177 stars 78 forks source link

Unneeded DATA_DIR kludge #349

Open greyhare opened 5 years ago

greyhare commented 5 years ago

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)
yakky commented 5 years ago

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!

greyhare commented 5 years ago

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.

yakky commented 5 years ago

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

greyhare commented 5 years ago

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

It would be a significant change, though.

eyadmba commented 4 years ago

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?

yakky commented 4 years ago

@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

eyadmba commented 4 years ago

Sounds awesome.

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

yakky commented 4 years ago

DATA_DIR is used to set MEDIA_ROOT, STATIC_ROOT

we will revise this upon the refactoring of the settings generation