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

Make isort fixer recognize auto_pipenv flag #3386

Merged
merged 8 commits into from
Jan 8, 2021

Conversation

ivorpeles
Copy link
Contributor

I noticed with python_auto_pipenv = 1 the black fixer would run on any file in a project containing a Pipfile whether vim was run inside the virtual environment or not. I wanted to make isort's fixer behave the same way in conjunction with the python_auto_pipenv flag.

What I have below generally works although I do have one concern: the !executable(l:executable) check fails when the executable ends up being pipenv. I added a hacky workaround to exclude this case but ideally there would be a test fixture that makes executable(pipenv) evaluate as true. We would expect this for any user setting python_auto_pipenv or python_isort_auto_pipenv to 1.

Would appreciate some feedback here, thanks!

Copy link
Contributor

@crimsonknave crimsonknave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love to see this! It appears that the PR adding similar function for black simply removed that section: https://github.com/dense-analysis/ale/pull/1988/files#diff-b9a6cf2d19edcb105ed741db26eed8b09f0be5348801b806e9dca68050255756L15-L17.


if !executable(l:executable)
if !executable(l:executable) && l:executable != 'pipenv'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current CI failure is complaining about != here: autoload/ale/fixers/isort.vim:27:50: Use robust operators !=#or!=?instead of!= (see Google VimScript Style Guide (Matching))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it also wanted isnot in place of !=, more robust when one of the operands isn't a string

@ivorpeles ivorpeles reopened this Nov 22, 2020
@ivorpeles
Copy link
Contributor Author

@crimsonknave Sorry for the massive delay here, took me a while to re-acquaint myself with whatever I was doing here. Tests are fixed and it works for me locally, let me know if anything else needs doing. Thanks!

@crimsonknave
Copy link
Contributor

@ivorpeles Thanks for fixing it up! I'm just a user of ALE, so I have no real idea what else if anything you need to do. I was just looking forward to this getting merged and took a look at what the black plugin did to see if I could help get it unstuck.

@ivorpeles
Copy link
Contributor Author

Oh cheers, thanks!

@ivorpeles
Copy link
Contributor Author

@w0rp didn't see anything in the contributing docs about review process, could I get a review on this?

@ivorpeles
Copy link
Contributor Author

@hsanson bumping for review, this has been open + passing tests for a while. Would be nice to get it merged

Copy link
Contributor

@hsanson hsanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really familiar with pipenv or how this is supposed to affect the fixed but comparing the changes with the black fixer seems to be correct.

@hsanson hsanson merged commit 54dd731 into dense-analysis:master Jan 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants