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

Use new lines option in Black 23.11 to format range #52

Merged
merged 6 commits into from
Dec 18, 2023

Conversation

remisalmon
Copy link
Contributor

@remisalmon remisalmon commented Dec 2, 2023

Possible fix for spyder-ide/spyder#21460 where formatting a text selection with black does not work if the code is intended due to black requiring the entire text as input.

This is using the new lines option added in black 23.11.0 that allows formatting only a range of lines in the text: https://github.com/psf/black/releases/tag/23.11.0

Doc:

Fixes #42

@@ -64,29 +64,33 @@ def pylsp_settings():


def format_document(client_config, document, range=None):
text = document.source
config = load_config(document.path, client_config)
lines = [(range["start"]["line"] + 1, range["end"]["line"])] if range else ()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Black says "indices are 1-based and inclusive on both ends" (https://black.readthedocs.io/en/stable/usage_and_configuration/the_basics.html#line-ranges), range uses a 0-based index.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also black says lines is a list of tuple but uses a tuple as default argument in https://github.com/psf/black/pull/4020/files.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Black says "indices are 1-based and inclusive on both ends" (https://black.readthedocs.io/en/stable/usage_and_configuration/the_basics.html#line-ranges), range uses a 0-based index.

Can you add this as a comment in the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just added.

@@ -1,2 +1,3 @@
a = 'hello'
b = 42
b = ["a", "very", "very", "very", "very", "very", "very", "very", "very", "long", "line"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not that long but long enough for black :) I added this line in the middle of the script to make sure we can format a selection in the middle of the text, that results in more lines in the final text.

@remisalmon
Copy link
Contributor Author

This should also fix #42.

@remisalmon remisalmon requested a review from haplo December 5, 2023 01:03
@ccordoba12
Copy link
Member

@remisalmon, please merge with master or rebase on top of it to get the changes in PR #53, which are necessary to increase our required Black version to 23.11.0

@ccordoba12 ccordoba12 added this to the v2.0.0 milestone Dec 5, 2023
@ccordoba12
Copy link
Member

@remisalmon, could you fix the small linting errors reported by Flake8 so we can merge this one? Thanks!

@remisalmon
Copy link
Contributor Author

@remisalmon, could you fix the small linting errors reported by Flake8 so we can merge this one? Thanks!

So this one is due to the long line formatting test and cannot be reformatted by black (this is very meta). I disabled the flake8 E501 line too long error on those 2 lines, let me know if that works.

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks @remisalmon for this great improvement!

@ccordoba12 ccordoba12 changed the title Use lines option to format range Use new lines option in Black 23.11 to format range Dec 18, 2023
@ccordoba12 ccordoba12 merged commit eb17580 into python-lsp:master Dec 18, 2023
5 checks passed
@ramnes
Copy link

ramnes commented Dec 19, 2023

That's awesome, thank you so much!

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.

Ineffective range formatting
4 participants