dense-analysis / ale

Check syntax in Vim/Neovim asynchronously and fix files, with Language Server Protocol (LSP) support
BSD 2-Clause "Simplified" License
13.48k stars 1.43k forks source link

Empty Pipfile files are created with Pipenv #2737

Open charlax opened 5 years ago

charlax commented 5 years ago

Information

VIM version

VIM - Vi IMproved 8.1 (2018 May 18, compiled Jul 21 2019 22:54:00)

Operating System: Mac

What went wrong

Empty Pipfile files are created alongside Python modules that are three subfolders deep from the projet's Pipfile.

See https://github.com/pypa/pipenv/issues/3908

Reproducing the bug

  1. Put in your config: let g:ale_python_auto_pipenv = 1
  2. Create three layers, e.g. a/b/c/foo.py
  3. Edit and save foo.py
  4. Observe that an empty a/b/c/Pipfile is created

:ALEInfo

Relevant section:

(finished - exit code 0) ['/usr/local/bin/zsh', '-c', 'cd ''[redacted]/api/api/task'' && ''pipenv'' run flake8 --format=default --stdin-display-name ''[redacted]/api/api/task/
connection.py'' - < ''/var/folders/ss/2cch6pjs7fqf55xztqstklh40000gn/T/vBtf28V/25/connection.py'''] 

Context

Pipenv will look for a Pipfile up to PIPENV_MAX_DEPTH (defaults to 3) up. If it does not find one, it will create an empty Pipfile.

A simple fix would be to use PIPENV_PIPFILE to provide an explicit path to the Pipfile.

charlax commented 5 years ago

Which probably requires creating a new function GetPipfilePath instead of just checking for Pipfile.lock presence in https://github.com/dense-analysis/ale/blob/master/autoload/ale/python.vim

ghost commented 5 years ago

Hi @charlax ,

Currently as you're probably aware, with g:ale_python_auto_pipenv=1, ALE searches upwards for Pipfile.lock to determine if pipenv is present for the project.

In this case once ALE has found pipenv, the issue your experiencing is dependent upon the working directory. If that is 3 directory levels deeper or more than the Pipfile then when ALE runs pipenv run ... which actually causes pipenv to create another Pipfile. I agree this is probably not the desired behaviour. Pipenv allows you to override this behaviour by setting the environment variable PIPENV_MAX_DEPTH extending the search depth.

Resolving this is a little tricky because the pipenv project has made some design decisions about the default behaviour of the tool and I think ALE should remain agnostic when affecting behaviours such that other ALE users don't get unexpected results.

I think best way forward would be to introduce:

g:ale_python_auto_pipenv_maxdepth (or b:ale_python_auto_pipenv_maxdepth)

When defined then ALE should use this integer value setting PIPENV_MAX_DEPTH during the call to pipenv.

Obviously, when the variable is not defined the standard pipenv functionality applies to maintain compatibility.

charlax commented 5 years ago

That would work, but I'm not sure why it would not be possible for ALE to look for the Pipfile in the current directory first... I assume most people open a file from the project root directory. This would take care of most cases without requiring a change to PIPENV_MAX_DEPTH.

ghost commented 5 years ago

Yes, it does, it starts in the current directory then searches upwards.

w0rp commented 5 years ago

Does setting the PIPENV_MAX_DEPTH environment variable appropriately outside of Vim, or in Vim with a $ variable fix the issue for you?

ghost commented 5 years ago

I've tried both which work.

The problem is if your WD is 3 levels or more from the Pipfile then pipenv, by default, will create a new Pipfile (also the side effect of taking longer to run the linter).

I'd say this is unexpected behaviour and might be surprising for ALE users.

Also .... oops I've already done the code 😇 ready for PR

w0rp commented 4 years ago

I closed that, see comments on the PR. If setting the environment variable will fix this, then let's update the documentation to describe how to set the variable. Maybe someone can also convince the pipenv authors to make the default maxdepth larger. 3 is too small.