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

Correctly format files and ranges with line endings other than LF #28

Merged
merged 3 commits into from
Mar 17, 2022

Conversation

ccordoba12
Copy link
Member

@ccordoba12 ccordoba12 commented Jan 7, 2022

  • black.format_file_contents (the function we're using to format files and ranges) fails for CRLF and CR. So we need to convert line endings to LF before using it and restore them afterwards.
  • I noticed this while I was investigating the bug reported in Black in Spyder 5 does not behave as standard Black spyder-ide/spyder#17143.
  • Drop support for Python 3.6 because this change requires python-lsp-server 1.4.0+, which also dropped support for that version.

@ccordoba12 ccordoba12 added this to the v1.1.0 milestone Jan 7, 2022
@ccordoba12 ccordoba12 requested a review from haplo January 7, 2022 17:06
@ccordoba12 ccordoba12 self-assigned this Jan 7, 2022
@ccordoba12 ccordoba12 changed the title Correctly format files with line endings other than LF Correctly format files and ranges with line endings other than LF Jan 7, 2022
pylsp_black/_utils.py Outdated Show resolved Hide resolved
@haplo
Copy link
Collaborator

haplo commented Jan 10, 2022

Thank you for the PR @ccordoba12, I will test it out as soon as I can, but I might not be able to do it until the weekend.

My initial thought is that I would expect black to handle EOL itself. But maybe that only works when black opens the file itself, but we are passing raw input text instead. I would like to understand the internals better.

I would also like to run some microbenchmarks for this code to make it as fast as possible, as it will be called all the time. The timeit module might be enough.

@ccordoba12 ccordoba12 modified the milestones: v1.1.0, v1.2.0 Jan 30, 2022
@ccordoba12
Copy link
Member Author

ccordoba12 commented Jan 30, 2022

Moving to 1.2.0 due to the urgency of releasing 1.1.0 given #29.

@ccordoba12 ccordoba12 force-pushed the fix-eols branch 2 times, most recently from 25e5887 to ac4e44f Compare March 15, 2022 16:53
Also drop support for Python 3.6 because that version of
python-lsp-server only supports 3.7+
@ccordoba12
Copy link
Member Author

@haplo, this is ready for review.

@@ -6,9 +6,11 @@
import black
import toml
from pylsp import hookimpl
from pylsp._utils import get_eol_chars
Copy link
Collaborator

@haplo haplo Mar 17, 2022

Choose a reason for hiding this comment

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

I wish the module was pylsp.utils and not _utils, but not much we can do about that at this point.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right. We inherited that from the old Palantir project and now it's used everywhere in our fork.

Copy link
Collaborator

@haplo haplo left a comment

Choose a reason for hiding this comment

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

LGTM 🙌

@ccordoba12
Copy link
Member Author

Thanks @haplo for the review!

Next week I'll work on the branch you created to add config options, so we can release 1.2.0 afterwards.

@ccordoba12 ccordoba12 merged commit 42005ae into python-lsp:master Mar 17, 2022
@ccordoba12 ccordoba12 deleted the fix-eols branch March 17, 2022 21:12
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.

2 participants