[WIP] Added black as a formatter #1234
Conversation
|
|
jarshwah
force-pushed
the
formatter-black
branch
from
b46ddb5 to
ae2a693
Compare
Codecov Report
@@ Coverage Diff @@
## master #1234 +/- ##
==========================================
+ Coverage 71.1% 72.37% +1.26%
==========================================
Files 269 261 -8
Lines 12395 11995 -400
Branches 2198 2135 -63
==========================================
- Hits 8814 8681 -133
+ Misses 3447 3190 -257
+ Partials 134 124 -10
Continue to review full report at Codecov.
|
| @@ -59,7 +59,7 @@ export abstract class BaseFormatter { | |||
| const executionInfo = this.helper.getExecutionInfo(this.product, args, document.uri); | |||
| executionInfo.args.push(tempFile); | |||
| const pythonToolsExecutionService = this.serviceContainer.get<IPythonToolExecutionService>(IPythonToolExecutionService); | |||
| const promise = pythonToolsExecutionService.exec(executionInfo, { cwd, throwOnStdErr: true, token }, document.uri) | |||
| const promise = pythonToolsExecutionService.exec(executionInfo, { cwd, throwOnStdErr: false, token }, document.uri) | |||
There was a problem hiding this comment.
Black outputs Reformatted <filename> to stderr for each file, which ends up breaking due to this. I think it'd be better to check the return code rather than inferring an error due to output on stderr - which is frequently used to communicate state rather than an error condition.
There was a problem hiding this comment.
Agreed. Though I don't remember why I did this. For some reason it was necessary. Please try testing it temporarily and test without the formatter installed. Ideally that should fail. If that works, my suggesting is to pass this as a flag to the base linter.
There was a problem hiding this comment.
I'm reluctant to change existing code, as I have a very vague memory of the fact that we couldn't rely on exit codes (at least for the linters).
There was a problem hiding this comment.
Thanks. I'll test with each of the formatters - both installed and not installed and see what happens.
|
|
||
| const args = ['--diff']; | ||
| // if (formatSelection) { | ||
| // black does not support partial formatting, throw an error? |
There was a problem hiding this comment.
Unsure how we should handle the lack of partial formatting.
There was a problem hiding this comment.
This is possible, all you need to do is, send in the source of the selected content.
Check the command line docs (black [OPTIONS] [SRC]...) - pass in the text
https://github.com/ambv/black#command-line-options
There was a problem hiding this comment.
FYI, @ambv has said he purposefully doesn't support partial formatting in Black so we might want to simply not support it in this instance.
There was a problem hiding this comment.
It wasn't possible to use stdin with the current way formatters are structured, without introducing some new concepts. I'm going to ignore partial formatting for now - maybe someone can attempt it in a future patch.
|
I'm closing this in favour of #1611 as I wanted to get this landed before PyCon US and took the opportunity to implement this from scratch as a learning exercise for myself. I do want to thank you, @jarshwah , for pushing us to get this support in. I'm also making sure you're listed as a co-author on my PR as I checked your PR once or twice to see how you handled something (although one of those times you solution was a comment of |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Not at all, thanks for taking over! Family has taken up most of my time for the last few weeks, so I'm glad at the outcome. I'll check out your PR too - interested in how you handled some of the issues. |
Fixes #1153
This is a work in progress PR. I only spent a little bit of time on it, but am sharing in case someone wants to pick up where I left off now that there is an issue open for it. I'm probably not going to have time to work on it in the next couple of weeks (away on holiday) but will pick back up when I'm back if no one else has taken over.
This pull request: