allo- / ffprofile

A tool to create firefox profiles with personalized defaults.
GNU Affero General Public License v3.0
769 stars 56 forks source link

Add an automatic install procedure #210

Closed Starli0n closed 3 years ago

Starli0n commented 3 years ago

Hi,

I created a Makefile (used as tasks launcher) to automatize the installation and run the server from the top folder.

I updated the documentation in Install.md. urls_project.py is the file that need to be replaced.

The only thing I did not manage to automatize is the editing of settings.py to add profilemaker, bootstrap3 and jquery.

I did not find in django documentation the way to add INTALLED_APPS in command line.

(I tested the procedure from scratch)

allo- commented 3 years ago

Using a Makefile is rather unconventional for python projects, but it looks fine.

You can probably append INSTALLED_APPS (with all three apps in it) to settings.py, as it is just a python variable to override the default, or append to it like a python list

For urls_project you can probably prepend (Usually the project routes should be loaded first) the include

urlpatterns = [
    url(r'', include("profilemaker.urls")),
] + urlpatterns

This would make the change a bit more robust against the admin script creating a changed urls.py in future django versions.

And please add a warning to the docs that venv and project are deleted (even on install). People may have a shared venv there, so it needs to be clear that the Makefile targets are for a clean debug environment and not for deploying the app.

Starli0n commented 3 years ago

Using a Makefile is rather unconventional for python projects, but it looks fine.

Yes, I admit that use it in most of my projects...

You can probably append INSTALLED_APPS (with all three apps in it) to settings.py, as it is just a python variable to override the default, or append to it like a python list

So I added the variable at the end of the file:

INSTALLED_APPS = [
    'django.contrib.admin',
    'django.contrib.auth',
    'django.contrib.contenttypes',
    'django.contrib.sessions',
    'django.contrib.messages',
    'django.contrib.staticfiles',
    'profilemaker',
    'bootstrap3',
    'jquery',
]

For urls_project you can probably prepend (Usually the project routes should be loaded first) the include This would make the change a bit more robust against the admin script creating a changed urls.py in future django versions.

I needed to add some includes as well, so the final file looks like this:

""" ... some comments ...
"""
from django.contrib import admin
from django.urls import path

urlpatterns = [
    path('admin/', admin.site.urls),
]

from django.conf.urls import include, url
from django.contrib.staticfiles.urls import staticfiles_urlpatterns

urlpatterns = [
    url(r'', include("profilemaker.urls")),
] + urlpatterns + staticfiles_urlpatterns()

And please add a warning to the docs that venv and project are deleted (even on install). People may have a shared venv there, so it needs to be clear that the Makefile targets are for a clean debug environment and not for deploying the app.

Finally I added the warning in the install procedure.

Everything works fine and was tested from scratch, so it is a complete automatic procedure :-)

allo- commented 3 years ago

Looks good. I currently cannot test myself, but I don't see obvious problems and when you tested it from scratch I just merge it and try it some other time.

Starli0n commented 3 years ago

Thank you for your trust 🙏