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.

Sign up for GitHub

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

Jump to bottom

Add diff support to blackd #969

Merged
merged 5 commits into from
Oct 28, 2019
Merged

Add diff support to blackd #969

merged 5 commits into from
Oct 28, 2019

Conversation

Jma353
Copy link
Contributor

@Jma353 Jma353 commented Aug 6, 2019

In this PR, I support to blackd equivalent to --diff for black. This was requested in #691.

I added a test. I also e2e tested it:

➜  black git:(jma353-add-diff-2-blackd) curl --data "print(   'hello, world'    )" localhost:45484 --header "X-Diff:true"
--- In	2019-08-06 01:01:42.622585 +0000
+++ Out	2019-08-06 01:01:42.630187 +0000
@@ -1 +1,2 @@
-print(   'hello, world'    )
+print("hello, world")
+

zsol
zsol reviewed Aug 7, 2019
View reviewed changes
blackd.py
src_name = f"In\t{then} +0000"
dst_name = f"Out\t{now} +0000"
loop = asyncio.get_event_loop()
formatted_str = await loop.run_in_executor(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we can just run black.diff synchronously here instead of delegating to a process pool.

For short files it's going to be probably faster, but if it takes too long it will block the event loop.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I convinced myself that this is probably fine for now, we can be more sophisticated later if it really is too slow

@zsol
Copy link
Collaborator

zsol commented Aug 7, 2019

Can you update the docs pls and write an entry in the change log?

@Jma353
Copy link
Contributor Author

Jma353 commented Aug 7, 2019

Thanks for the feedback @zsol, will push a revision later today.

@ambv
Copy link
Collaborator

ambv commented Oct 20, 2019

If you hurry this will get to the next beta.

ambv
ambv requested changes Oct 20, 2019
View reviewed changes
Copy link
Collaborator

@ambv ambv left a comment

Choose a reason for hiding this comment

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

Marking as request changes so I can filter through the PRs easier.

@Jma353
Copy link
Contributor Author

Jma353 commented Oct 20, 2019

I can revise this by EOD. Sorry for letting this sit, got busy with some other side-projects recently :).

@ambv
Copy link
Collaborator

ambv commented Oct 20, 2019

Never apologize for voluntary contributions you're making for free in your spare time. We are thankful for any level of commitment you can provide.

@Jma353
Copy link
Contributor Author

Jma353 commented Oct 22, 2019

Delayed by pip install issues with pip install regex==2019.08.19. Fails on my Mac for some reason.

@Jma353
Copy link
Contributor Author

Jma353 commented Oct 27, 2019

Should be good to go, added changelog + doc for the new header flag.

@ambv ambv merged commit df6e1a4 into psf:master Oct 28, 2019
@ambv
Copy link
Collaborator

ambv commented Oct 28, 2019

Thanks! ✨ 🍰 ✨

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zsol zsol zsol left review comments

@ambv ambv ambv requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Jma353 @zsol @ambv