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

Add support for black usage with pipenv #1988

Merged
merged 5 commits into from
Jan 3, 2019

Conversation

mdtusz
Copy link
Contributor

@mdtusz mdtusz commented Oct 10, 2018

For many projects and just out of habit, I have black installed using pipenv - the standard black executable will work assuming pipenv shell has been run before opening vim, but I often forget or haven't run it.

This adds support for black usage with pipenv the same way that it works with mypy and other python tools.

autoload/ale/fixers/black.vim Outdated Show resolved Hide resolved
@RyanSquared
Copy link
Member

[00:01:01]   Starting Vader: C:\testplugin\test\fixers\test_black_fixer_callback.vader
[00:01:01]     (1/3) [EXECUTE] The black callback should return the correct default values
[00:01:01]     (1/3) [EXECUTE] (X) Vim(if):E735: Can only compare Dictionary with Dictionary
[00:01:01]     (2/3) [EXECUTE] The black callback should include options
[00:01:01]     (3/3) [EXECUTE] Pipenv is detected when python_black_auto_pipenv is set
[00:01:01]     (3/3) [EXECUTE] (X) {'command': 'xxxinvalid -'} should be equal to {'command': 'pipenv run mypy'}
[00:01:01]   Success/Total: 1/3

Latest failed segment.

RyanSquared
RyanSquared previously approved these changes Oct 25, 2018
Copy link
Member

@w0rp w0rp left a comment

Choose a reason for hiding this comment

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

I just noticed an issue here.


let l:options = ale#Var(a:buffer, 'python_black_options')

return {
\ 'command': ale#Escape(l:executable)
\ 'command': ale#Escape(l:executable. l:exec_args)
Copy link
Member

Choose a reason for hiding this comment

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

This isn't right. This will produce a command string like "pipenv run black" -. You want something more like "pipenv" run black -. You should move . l:exec_args outside of the ale#Escape call.

call ale#test#SetFilename('/testplugin/test/python_fixtures/pipenv/whatever.py')

AssertEqual
\ {'command': ale#Escape('pipenv run black') . ' -'},
Copy link
Member

Choose a reason for hiding this comment

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

Fix the test here, given the suggestion above.

@RyanSquared RyanSquared dismissed their stale review October 25, 2018 15:43

Missed potential bug

@RyanSquared
Copy link
Member

@mdtusz do you mind fixing some things relating to w0rp's review? After that, it should be good to merge.

@mdtusz
Copy link
Contributor Author

mdtusz commented Dec 3, 2018

Sorry for the radio silence! Will try to get around to it this week to close it up!

@mdtusz
Copy link
Contributor Author

mdtusz commented Dec 22, 2018

Sorry for the extreme delays! I've fixed up the tests, but vimscript/vader is far out of my normal wheelhouse so let me know if anything looks suspect.

@w0rp w0rp merged commit a1654cf into dense-analysis:master Jan 3, 2019
@w0rp
Copy link
Member

w0rp commented Jan 3, 2019

Cheers! 🍻

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