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

Unneeded DATA_DIR kludge #349

Open
greyhare opened this issue Jun 30, 2019 · 8 comments
Open

Unneeded DATA_DIR kludge #349

greyhare opened this issue Jun 30, 2019 · 8 comments

Comments

@greyhare
Copy link

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
Copy link
Member

yakky commented Jul 2, 2019

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
Copy link
Author

greyhare commented Jul 4, 2019

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
Copy link
Member

yakky commented Jul 4, 2019

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
Copy link
Author

greyhare commented Jul 4, 2019

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

It would be a significant change, though.

@eyadmba
Copy link

eyadmba commented Jan 2, 2020

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
Copy link
Member

yakky commented Jan 2, 2020

@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
Copy link

eyadmba commented Jan 4, 2020

Sounds awesome.

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

@yakky
Copy link
Member

yakky commented Jan 4, 2020

DATA_DIR is used to set MEDIA_ROOT, STATIC_ROOT

we will revise this upon the refactoring of the settings generation

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

No branches or pull requests

3 participants