search

jorgenschaefer / elpy

Emacs Python Development Environment
GNU General Public License v3.0
1.89k stars 260 forks source link

Incorrect check for Django settings module #1340

Open felipeochoa opened 6 years ago

felipeochoa commented 6 years ago

Summary

1291 broke elpy-test for my Django project. My project prints a single line to stdout while loading the logging configuration, so this check incorrectly reports that the module cannot be imported.

Steps to reproduce

  1. Create a new Django project
  2. Add print("hi") to your settings file
  3. Run elpy-test in that project

Result of (elpy-config)

Virtualenv........: iq-dist (~/.venv)
RPC Python........: 3.6.2 (~/.venv/bin/python)
Interactive Python: python (~/.venv/bin/python)
Emacs.............: 25.2.1
Elpy..............: 1.19.0
Jedi..............: 0.11.1
Rope..............: Not found (0.10.7 available)
Autopep8..........: 1.3.4 (1.3.5 available)
Yapf..............: 0.21.0
Syntax checker....: flake8 (~/.venv/bin/flake8)

I don't have time right now to submit a PR, but I think changing the check to be:

(zerop (process-file-shell-command
                       (format "%s -c 'import %s'" elpy-rpc-python-command django-settings-env)))

should fix this issue

felipeochoa commented 6 years ago

Actually, even fixing this doesn't resolve the core issue since the following s-trim call will still fail.

And there a couple more issues around here:

felipeochoa commented 6 years ago

Here's a better version of get-test-runner:

(defun elpy-django--get-test-runner ()
  "Return the name of the django test runner.
Needs `DJANGO_SETTINGS_MODULE' to be set in order to work."
  (let* ((process-environment (append compilation-environment process-environment))
         (django-import-cmd "import django;django.setup();from django.conf import settings;print(settings.TEST_RUNNER, end=\"\")")
         (django-settings-env (getenv "DJANGO_SETTINGS_MODULE"))
         (default-directory (elpy-project-root)))
    ;; If no Django settings has been set, then nothing will work. Warn user
    (when (not django-settings-env)
      (error "Please set environment variable `DJANGO_SETTINGS_MODULE' if you'd like to run the test runner"))

    (with-temp-buffer
      ;; We have to be able to import the DJANGO_SETTINGS_MODULE
      ;; A non-zero exit code will indicate an error importing django settings
      (unless (zerop (process-file-shell-command
                      (format "%s -c '%s'" elpy-rpc-python-command django-import-cmd)
                      nil '(t nil)))
        (error (format "Unable to import DJANGO_SETTINGS_MODULE: '%s'" django-settings-env)))
      ;; Return the last line of output, which has the fully qualified name of the test runner to use
      (forward-line 0)
      (buffer-substring-no-properties (point) (point-max)))))

Sorry I can't package this into a PR with tests, but hopefully this helps solve the issue. This also has the benefit of only shelling out to python once

gopar commented 6 years ago

Thanks for the snippet, i'll look at it on the weekend.