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 range formatting to the ruby-lsp #203

Closed
vinistock opened this issue Jul 15, 2022 · 5 comments · Fixed by #2657
Closed

Add range formatting to the ruby-lsp #203

vinistock opened this issue Jul 15, 2022 · 5 comments · Fixed by #2657
Labels
enhancement New feature or request pinned This issue or pull request is pinned and won't be marked as stale

Comments

@vinistock
Copy link
Member

Range formatting is a request very similar to formatting, but it applies only to the selected range and not the entire document.

Currently, trying to format ranges with either RuboCop or SyntaxTree fails, because both believe they are always looking at whole files instead of ranges.

For RuboCop, this will be difficult to overcome, given that trying to format a range in the middle of the document will even add # frozen_string_literal comments.

However, we may be able to use SyntaxTree for this if we add the ability of informing the formatter about the current indentation (or nesting) level. In the current behaviour, the code inside the range will be considered as a whole file and everything is pushed to indentation level zero.

Documentation: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_rangeFormatting

@vinistock vinistock mentioned this issue Jul 15, 2022
@vinistock vinistock added enhancement New feature or request pinned This issue or pull request is pinned and won't be marked as stale labels Jul 20, 2023
andyw8 pushed a commit to andyw8/ruby-lsp that referenced this issue Mar 2, 2024
…sce-2.11.0

Bump vsce from 2.10.2 to 2.11.0
@bsgreenb
Copy link

This would be big! Can't use formatOnSaveMode until this is introduced!

@choosen
Copy link

choosen commented Sep 30, 2024

would it be easier for VSCode addon to send whole file to language server and then filter response linter errors for modified only lines ?

// I don't know how it works under the hood.

Overcommit + rubocop had option problem_on_unmodified_line: warn to skip others

@vinistock
Copy link
Member Author

@choosen RuboCop has no API to get corrections for only a portion of the file. Filtering diagnostics for a section of the file is relatively straight forward, but that's unrelated to formatting. Since in formatting we only get the formatted string back, it would not be possible to know which corrections have to be excluded.

RuboCop would need an API that allows for partial formatting and cops would need to know if they are operating on the entire file or not.

For example, if you're auto-correcting only a section of a file, you don't want the # frozen_string_literal: true comment to be added. And you would like the indentation of that section to be considered as the starting point for all corrections, otherwise it would bring all code to zero indentation even when applying formatting in the middle of a class.

Consider this example, which is what would currently happen without a new API being introduced to RuboCop.

# Example: before formatting
# frozen_string_literal: true

class Foo
  def initialize
  end
end

# ----
# Example: after formatting
# frozen_string_literal: true

class Foo
# frozen_string_literal: true
def initialize
end
end

For reference, I added support for this to Syntax Tree, but it's easier because Syntax Tree is a pure formatter and doesn't perform linting like RuboCop does.

@choosen
Copy link

choosen commented Oct 1, 2024

@vinistock use case of modifying # frozen_string_literal: true

is about sending par of the inspected file to rubocop.

I said that you send whole file , parse rubocop output and keep only modified lines.
then frozen string literal will not be a problem as it would be added on top of the file and will be ignored as you did not change file top (it will work only on new files)

Maybe some problems will be with whole function definition indentation, but also better to have partial rubocop rules setup of linter in terms of modification range than nothing or whole file ?

@vinistock
Copy link
Member Author

vinistock commented Oct 2, 2024

My concern is whether that approach will be brittle or not and the performance of doing more work on top of the formatting (which is already not very fast).

Additionally, RuboCop allows for extensions (rubocop-performance, rubocop-rails, ...) and it's not clear to me that we can ensure all of the corrections will be only what's relevant for the selected range.

I put up #2657 to add the request implementation for Syntax Tree, which I added the bits needed for range formatting myself.

If you have an idea of how to make it work for RuboCop, then please propose a PR that will implement run_range_formatting in lib/ruby_lsp/requests/support/rubocop_formatter.rb.

Maybe some problems will be with whole function definition indentation, but also better to have partial rubocop rules setup of linter in terms of modification range than nothing or whole file ?

Based on my experience working on the Ruby LSP for over 2 years, I can tell you with confidence that if we format the files incorrectly people will be a lot more upset than if we simply don't provide the feature 😅. My concern is applying incorrect edits to the range, which I couldn't find a general solution for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request pinned This issue or pull request is pinned and won't be marked as stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants