-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Blacken the code base #27076
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
Blacken the code base #27076
Conversation
dfcda1c
to
f955bf3
Compare
89f9547
to
fa4ebbb
Compare
fa4ebbb
to
cb7c4f7
Compare
@TomAugspurger a question we are having here at the sprint, is that if you add it as a pre-commit hook, does it run black on the full code base (as that takes a while for pandas) ? |
Nope it's just the changed files.
…On Thu, Jun 27, 2019 at 1:48 PM Joris Van den Bossche < ***@***.***> wrote:
@TomAugspurger <https://github.com/TomAugspurger> a question we are
having here at the sprint, is that if you add it as a pre-commit hook, does
it run black on the full code base (as that takes a while for pandas) ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#27076?email_source=notifications&email_token=AAKAOIXIDQPXLVW26PM3XD3P4UDQNA5CNFSM4H35A6AKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYYBDHY#issuecomment-506466719>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAKAOIU5RYHBUPHVTUBDKW3P4UDQNANCNFSM4H35A6AA>
.
|
7c51d0b
to
4d30876
Compare
The test with 79 limit added around 20,000 lines |
@simonjayhawkins is the black default of 88 too wide? |
I'm happy to go with the black default and see how it goes, if nobody else is objecting strongly.
maybe too much nested code! |
@jorisvandenbossche should prob merge this right after the RC is cut |
Would we consider doing it before? Seems like we’d get better test coverage of it if cut with the initial RC
…Sent from my iPhone
On Jul 3, 2019, at 2:47 PM, Jeff Reback ***@***.***> wrote:
@jorisvandenbossche should prob merge this right after the RC is cut
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
41859ee
to
2b7e27e
Compare
@jorisvandenbossche sorry just merged the last 2 PRs |
No problem, the main work here was updating the fixup commits and checking they are still fine for flake8 / isort. Once we actually put it in, I can just cherry-pick them on top of the latest blackened one. |
Do we want to do this now? (after the RC is fine for me as well) @TomAugspurger are you still planning to tag tonight? |
cool, I would say fix this up and let's push it in! |
Yep, fine with doing this now. |
Do we also want to blacken the asv benchmarks ? (currently I only did the actual pandas package) Re-applying it now |
i think blacken everything as we flake everything |
@jorisvandenbossche fixed the conflicts, reformatted, and fixed the lint errors in https://github.com/pandas-dev/pandas/runs/161581700 |
Ah, sorry, have them locally as well. Did you also fix the test_validate_docstrings error? That I am doing now (and also ran black on the full repo instead of only on /pandas) |
Ah, sorry. I missed the validate_docstrings one. Feel free to force push. |
class TimedeltaConstructor:
diff --git a/pandas/core/indexes/datetimelike.py b/pandas/core/indexes/datetimelike.py
index f2e6f631ae9..731ab9c4163 100644
--- a/pandas/core/indexes/datetimelike.py
+++ b/pandas/core/indexes/datetimelike.py
@@ -73,14 +73,14 @@ class DatetimeIndexOpsMixin(ExtensionOpsMixin):
# properties there. They can be made into cache_readonly for Index
# subclasses bc they are immutable
inferred_freq = cache_readonly(
- DatetimeLikeArrayMixin.inferred_freq.fget
- ) # type: ignore
+ DatetimeLikeArrayMixin.inferred_freq.fget # type: ignore
+ )
_isnan = cache_readonly(DatetimeLikeArrayMixin._isnan.fget) # type: ignore
hasnans = cache_readonly(DatetimeLikeArrayMixin._hasnans.fget) # type: ignore
_hasnans = hasnans # for index / array -agnostic code
_resolution = cache_readonly(
- DatetimeLikeArrayMixin._resolution.fget
- ) # type: ignore
+ DatetimeLikeArrayMixin._resolution.fget # type: ignore
+ )
resolution = cache_readonly(DatetimeLikeArrayMixin.resolution.fget) # type: ignore
_maybe_mask_results = ea_passthrough(DatetimeLikeArrayMixin._maybe_mask_results) seemed to fix the typing issue. |
BTW, are you OK with re-enabling the flake8 check for whitespace after comma? I first put that in the exclude list, as black can generate that (eg in |
f896dc7
to
568c0e7
Compare
Up to now I kept separate commits for updating CI, the actual reformatting, and some fix-ups (because it made re-applying easier). Do we want to keep those separate when merging to keep the automated changes in a separate commit? (or it might be fine to have the separate commits here in the PR) |
Multiple commits (rebase and merge) seems fine. |
@jorisvandenbossche all green. |
568c0e7
to
1efaa2b
Compare
I just rebased to update the commit titles and to combine the last two fixup commits (so they should be fine to rebase/merge). They should still be exactly the same content, so relying on green CI of the previous push. |
Thanks! I'll need to give a bottle before pushing the tag. I'll check the
build on master when it finishes.
…On Wed, Jul 3, 2019 at 10:28 PM Joris Van den Bossche < ***@***.***> wrote:
Merged #27076 <#27076> into
master.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#27076?email_source=notifications&email_token=AAKAOITO5K4GHDQZQBZNCU3P5VU6HA5CNFSM4H35A6AKYY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOSKLC3UY#event-2459315667>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAKAOIVZPMY7AWSLQRHNBDTP5VU6HANCNFSM4H35A6AA>
.
|
So we can see what it looks like