CI: Use black for code formatting #237
Conversation
|
If we do decide to go forward with this, we should wait till large outstanding PRs are in. This branch will be relatively easy to update. |
Codecov Report
@@ Coverage Diff @@
## master #237 +/- ##
========================================
Coverage ? 93.2%
========================================
Files ? 30
Lines ? 1677
Branches ? 0
========================================
Hits ? 1563
Misses ? 114
Partials ? 0
Continue to review full report at Codecov.
|
|
Also added coverage to the py36 build (turned off comments for now). |
dask_ml/_partial.py
Outdated
| kwargs, | ||
| ) | ||
| for i in range(nblocks) | ||
| } |
There was a problem hiding this comment.
I think I'm for black (it'd save me time), but this one-element-per-line is one formatting option I dislike, especially for many options.
There was a problem hiding this comment.
Black does have support for # fmt: off and # fmt: on flags to stop Black's formatting. I think we should use those if Black is used.
There was a problem hiding this comment.
Hmm... Agreed that this is less pleasant.
However, I'm inclined to do all or nothing here. Locally, I would set up a pre-commit hook and then never worry about it again :)
|
+1 on all or nothing
…On Thu, Jun 28, 2018 at 7:34 AM, Tom Augspurger ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In dask_ml/_partial.py
<#237 (comment)>:
> dsk = {(name, -1): model}
- dsk.update({(name, i): (_partial_fit, (name, i - 1),
- (x.name, i, 0),
- (getattr(y, 'name', ''), i), kwargs)
- for i in range(nblocks)})
+ dsk.update(
+ {
+ (name, i): (
+ _partial_fit,
+ (name, i - 1),
+ (x.name, i, 0),
+ (getattr(y, "name", ""), i),
+ kwargs,
+ )
+ for i in range(nblocks)
+ }
Hmm... Agreed that this is less pleasant.
However, I'm inclined to do all or nothing here. Locally, I would set up a
pre-commit hook
<https://github.com/ambv/black#version-control-integration> and then
never worry about it again :)
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#237 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AASszKixk4uhNAD4W7axHTf5KxXeVKvxks5uBL84gaJpZM4U6MjO>
.
|
|
@stsievert will wait for your +/- 1 here. Will also wait for large PRs (hypberband) to be merged first, if we go ahead with it. |
|
👍, though that's dependent on being able to use |
|
Should we add black's
Maybe not. Not having to think about formatting would be really nice. |
https://github.com/dask/dask-ml/pull/237/files#diff-380c6a8ebbbce17d55d50ef17d3cf906 has the changes to linting settings so that flake8 still passes.
That's the hope :) |
|
Rebased on master. Apologies for the force push, but I think having a clean git commit history of
is useful here. I'd like to merge this today if possible. I'll follow up with commits to outstanding PRs to use the new style. |
|
Here goes. |
|
Whoops, I just realized that I blew away the update to |
Just as an exercise to see if this would work.
I'm 50/50 on whether this is a good idea or not.