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

Range formatting support #373

Merged
merged 3 commits into from
Feb 5, 2024
Merged

Range formatting support #373

merged 3 commits into from
Feb 5, 2024

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Jan 31, 2024

Summary

This PR implements support for range formatting for Python files. It requires astral-sh/ruff#9733

Note to myself: Writing Python is hard 😅

Closes #116
Closes astral-sh/ruff-vscode#319

Limitations

Range formatting Notebooks isn't supported because it raises a few interesting questions:

  • Should the range be relative to the concatenated cell content?
  • Should the range use a custom notation cell:offset
  • What if the range falls on the boundary of two cells?

I feel like implementing range formatting will be straightforward with a Rust LSP because:

  • We can make the assumption that only ever a single cell gets formatted (solves some of the problems above)
  • We can directly call into the range formatting API without having to find the proper CLI abstractions.

I tried to teach VS Code that Range formatting isn't supported for notebooks by using a textSelector but VS code doesn't seem to care :(
That's why the implementation defaults to format the entire cell for now. I'm open to suggestions.

Test Plan

ruff_lsp/server.py Outdated Show resolved Hide resolved
ruff_lsp/server.py Outdated Show resolved Hide resolved
Comment on lines +1229 to +1257
# We don't support range formatting of notebooks yet but VS Code
# doesn't seem to respect the document filter. For now, format the entire cell.
range = None if document.kind is DocumentKind.Cell else range
Copy link
Member

Choose a reason for hiding this comment

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

This is unfortunate because as a user I wouldn't expect that "Format selection" in Jupyter notebook would format an entire cell. I'm not sure what the right solution is but I'd prefer to send a info / warning message to the user stating that range formatting isn't yet supported for Jupyter Notebooks instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree but I wasn't able to come up with something better. I'm worried that a warning would be rather annoying and force users to change their setting from modificationsIfAvailable to the entire document or they get annoyed by the warning on every save

@MichaReiser MichaReiser changed the base branch from main to narrow-edits February 2, 2024 17:12
Base automatically changed from narrow-edits to main February 2, 2024 17:20
@MichaReiser MichaReiser force-pushed the range-formatting branch 2 times, most recently from 3d3ecda to 7431410 Compare February 2, 2024 17:23
@MichaReiser MichaReiser merged commit 80878b9 into main Feb 5, 2024
20 checks passed
@MichaReiser MichaReiser deleted the range-formatting branch February 5, 2024 19:20
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.

Support format modificationsIfAvailable Investigate range formatting
4 participants