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

Add support for LSP formatting options parameter #134

Merged
merged 11 commits into from
Apr 5, 2022

Conversation

masad-frost
Copy link
Contributor

This PR adds support for LSP formatting options https://microsoft.github.io/language-server-protocol/specifications/specification-3-17/#formattingOptions for the yapf

It adds explicit support for tabSize and insertSpaces, the rest of the options are not configurable in yapf. I did however add support for arbitrary configurations as specified in [key: string]: boolean | integer | string;.

I think this PR might break plugins as the API now passes options to formatter hooks, but not sure.

Nits are welcome as I don't normally read/write python.

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.

Hey @masad-frost, thanks for your contribution! Please read my comment below to understand how to reorient your work to fit the design of the other plugins in this project.

pylsp/hookspecs.py Show resolved Hide resolved
@krassowski
Copy link
Contributor

The tests are failing on CI:

>           style_config = style.CreateStyleFromConfig(style_config)
E           NameError: name 'style' is not defined

@masad-frost
Copy link
Contributor Author

Lint caught an issue where I wasn't passing options for range formatting!

@ccordoba12
Copy link
Member

ccordoba12 commented Mar 30, 2022

Hey @masad-frost, I fixed the linting errors reported here in another PR of mine.

Please let me know if you need help to fix the merge conflicts so we can merge this one.

@masad-frost
Copy link
Contributor Author

Done

@masad-frost
Copy link
Contributor Author

Hmm, not sure why lint passed locally. Hopefully this is the last of it

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.

Hey @masad-frost, last review then this should be ready.

I've also seen that you have struggled a lot with our linting tests, so I'll give you a hand with them if you're unable to fix them after this review.

pylsp/plugins/autopep8_format.py Outdated Show resolved Hide resolved
pylsp/plugins/autopep8_format.py Outdated Show resolved Hide resolved
test/plugins/test_yapf_format.py Outdated Show resolved Hide resolved
@ccordoba12 ccordoba12 added this to the v1.5.0 milestone Apr 4, 2022
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 for your contribution @masad-frost!

@ccordoba12 ccordoba12 changed the title Add support for LSP formatting options parameter Add support for LSP formatting options parameter Apr 5, 2022
@ccordoba12 ccordoba12 merged commit 4bffc39 into python-lsp:develop Apr 5, 2022
@masad-frost masad-frost deleted the fm-yapf-formatting-options branch May 11, 2022 17:06
@masad-frost
Copy link
Contributor Author

Hey @ccordoba12, when do you plan on tagging a new release? Wondering because we wanna upgrade, also maybe i can slip #136 before then.

@ccordoba12
Copy link
Member

Perhaps in a month or so.

@masad-frost
Copy link
Contributor Author

Good time to bump to 1.5?

@ccordoba12
Copy link
Member

Yep, I'll release it next week.

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.

None yet

3 participants