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

Make RuboCop runner composable and capture any stdout message from it #275

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
44 changes: 13 additions & 31 deletions lib/ruby_lsp/requests/support/rubocop_diagnostics_runner.rb
Original file line number Diff line number Diff line change
@@ -1,11 +1,8 @@
# typed: strict
# frozen_string_literal: true

begin
require "rubocop"
rescue LoadError
return
end
require "ruby_lsp/requests/support/rubocop_runner"
return unless defined?(::RubyLsp::Requests::Support::RuboCopRunner)

require "cgi"
require "singleton"
Expand All @@ -14,46 +11,31 @@ module RubyLsp
module Requests
module Support
# :nodoc:
class RuboCopDiagnosticsRunner < RuboCop::Runner
class RuboCopDiagnosticsRunner
extend T::Sig
include Singleton

sig { void }
def initialize
@options = T.let({}, T::Hash[Symbol, T.untyped])
@uri = T.let(nil, T.nilable(String))
@diagnostics = T.let([], T::Array[Support::RuboCopDiagnostic])

super(
::RuboCop::Options.new.parse([
"--stderr", # Print any output to stderr so that our stdout does not get polluted
"--force-exclusion",
"--format",
"RuboCop::Formatter::BaseFormatter", # Suppress any output by using the base formatter
]).first,
::RuboCop::ConfigStore.new
)
@runner = T.let(RuboCopRunner.new, RuboCopRunner)
end

sig { params(uri: String, document: Document).returns(T::Array[Support::RuboCopDiagnostic]) }
def run(uri, document)
@diagnostics.clear
@uri = uri

file = CGI.unescape(URI.parse(uri).path)
# We communicate with Rubocop via stdin
@options[:stdin] = document.source

filename = CGI.unescape(URI.parse(uri).path)
# Invoke RuboCop with just this file in `paths`
super([file])
@diagnostics
@runner.run(filename, document.source)

diagnostics(uri)
end

private

sig { params(_file: String, offenses: T::Array[RuboCop::Cop::Offense]).void }
def file_finished(_file, offenses)
@diagnostics = offenses.map { |offense| Support::RuboCopDiagnostic.new(offense, T.must(@uri)) }
sig { params(uri: String).returns(T::Array[Support::RuboCopDiagnostic]) }
def diagnostics(uri)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure there's a lot of value in keeping this method around after the refactor. What do you think about moving this to the run body?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, we can do that!

@runner.offenses.map do |offense|
Support::RuboCopDiagnostic.new(offense, uri)
end
end
end
end
Expand Down
33 changes: 10 additions & 23 deletions lib/ruby_lsp/requests/support/rubocop_formatting_runner.rb
Original file line number Diff line number Diff line change
@@ -1,11 +1,8 @@
# typed: strict
# frozen_string_literal: true

begin
require "rubocop"
rescue LoadError
return
end
require "ruby_lsp/requests/support/rubocop_runner"
return unless defined?(::RubyLsp::Requests::Support::RuboCopRunner)

require "cgi"
require "singleton"
Expand All @@ -14,35 +11,25 @@ module RubyLsp
module Requests
module Support
# :nodoc:
class RuboCopFormattingRunner < RuboCop::Runner
class RuboCopFormattingRunner
extend T::Sig
include Singleton

sig { void }
def initialize
@options = T.let({}, T::Hash[Symbol, T.untyped])

super(
::RuboCop::Options.new.parse([
"--stderr", # Print any output to stderr so that our stdout does not get polluted
"--force-exclusion",
"--format",
"RuboCop::Formatter::BaseFormatter", # Suppress any output by using the base formatter
"-a", # --auto-correct
]).first,
::RuboCop::ConfigStore.new
)
@runner = T.let(RuboCopRunner.new(
"-a", # --auto-correct
), RuboCopRunner)
end

sig { params(uri: String, document: Document).returns(T.nilable(String)) }
def run(uri, document)
file = CGI.unescape(URI.parse(uri).path)
# We communicate with Rubocop via stdin
@options[:stdin] = document.source
filename = CGI.unescape(URI.parse(uri).path)

# Invoke RuboCop with just this file in `paths`
super([file])
@options[:stdin]
@runner.run(filename, document.source)

@runner.formatted_source
end
end
end
Expand Down
90 changes: 90 additions & 0 deletions lib/ruby_lsp/requests/support/rubocop_runner.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
# typed: strict
# frozen_string_literal: true

begin
require "rubocop"
rescue LoadError
return
end

require "cgi"
require "singleton"

module RubyLsp
module Requests
module Support
# :nodoc:
class RuboCopRunner < RuboCop::Runner
extend T::Sig

sig { returns(T::Array[RuboCop::Cop::Offense]) }
attr_reader :offenses

DEFAULT_ARGS = T.let([
"--force-exclusion",
Copy link
Member

Choose a reason for hiding this comment

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

I assume we dropped the stderr option because we're now capturing stdout, but should we keep the formatter to reduce the amount of prints from RuboCop?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we can still keep things less chatty overall. I'll reinstate.

].freeze, T::Array[String])

sig { params(args: String).void }
def initialize(*args)
@options = T.let({}, T::Hash[Symbol, T.untyped])
@offenses = T.let([], T::Array[RuboCop::Cop::Offense])
@errors = T.let([], T::Array[String])
@warnings = T.let([], T::Array[String])

args += DEFAULT_ARGS
rubocop_options = ::RuboCop::Options.new.parse(args).first
super(rubocop_options, ::RuboCop::ConfigStore.new)
end

sig { params(path: String, contents: String).void }
def run(path, contents)
@errors = []
@warnings = []
Comment on lines +41 to +42
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need these?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since we don't recycle runners, but only use a single instance, errors and warnings need to cleared at the top of every run so that they don't keep building up run over run.

@offenses = []
Copy link
Member

Choose a reason for hiding this comment

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

Can we use @offenses.clear 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.

That was what I tried first, but it didn't work since it is a frozen array.

@options[:stdin] = contents
capture_output { super([path]) }
display_handled_errors
end

sig { returns(String) }
def formatted_source
@options[:stdin]
end

private

sig { void }
def display_handled_errors
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this? What type of error is this trying to display? If we don't pop up a dialogue, I doubt anyone will actually see it.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is so that people can see and/or report problems that any formatting/diagnostic operation has encountered. I feel like this is needed more for troubleshooting than making people aware. After all, there is no action to be taken on any of these, since they are already handled.

This is what the output looks like:

[RuboCop] Encountered and handled errors:
[RuboCop]   - An error occurred while Layout/BlockEndNewline cop was inspecting /Users/ufuk/src/github.com/Shopify/tapioca/lib/tapioca/static/rbs/parameter_converter.rb:4:17.

return if errors.empty?

$stderr.puts "[RuboCop] Encountered and handled errors:"
errors.uniq.each do |error|
$stderr.puts "[RuboCop] - #{error}"
end
end

sig { params(_file: String, offenses: T::Array[RuboCop::Cop::Offense]).void }
def file_finished(_file, offenses)
@offenses = offenses
end

sig { params(block: T.proc.void).void }
def capture_output(&block)
original_verbosity = $VERBOSE
orig_stdout = $stdout
orig_stderr = $stderr

$VERBOSE = nil
$stderr = StringIO.new
Copy link
Contributor

Choose a reason for hiding this comment

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

How hard/bad would it be to monkey patch the classes from Rubocop that still print so we noop puts rather than changing the global variable?

$stdout = StringIO.new

block.call
ensure
$stdout = orig_stdout
$stderr = orig_stderr
$VERBOSE = original_verbosity
Comment on lines +83 to +85
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, VERBOSE is first in the two previous related blocks.

Suggested change
$stdout = orig_stdout
$stderr = orig_stderr
$VERBOSE = original_verbosity
$VERBOSE = original_verbosity
$stdout = orig_stdout
$stderr = orig_stderr

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 was explicitly reverting changes in the reverse order :)

end
end
end
end
end