From 56d2bf0223dae2aed27e4d1d4d4f9c105271b003 Mon Sep 17 00:00:00 2001 From: schneems Date: Thu, 18 Nov 2021 15:45:42 -0600 Subject: [PATCH 1/4] Add interface to require dead_end without core_ext Currently `dead_end` works by monkey patching `require` and friends. If someone wants to programmatically execute dead_end manually via `DeadEnd.handle_error` without monkey patches they can now do that by setting the environment variable `DISABLE_DEAD_END_CORE_EXT=1`. --- CHANGELOG.md | 4 +++ README.md | 10 +++++++ lib/dead_end.rb | 35 +++++++++++++++++++--- lib/dead_end/auto.rb | 33 ++------------------ lib/dead_end/core_ext.rb | 33 ++++++++++++++++++++ spec/integration/dead_end_spec.rb | 16 ++++++++++ spec/integration/ruby_command_line_spec.rb | 6 ++++ 7 files changed, 102 insertions(+), 35 deletions(-) create mode 100644 lib/dead_end/core_ext.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index daa967c..14d574b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ ## HEAD (unreleased) +- Requiring `dead_end/auto` is now deprecated please require `dead_end` instead (https://github.com/zombocom/dead_end/pull/119) +- The interface `DeadEnd.handle_error` is declared public and stable (https://github.com/zombocom/dead_end/pull/119) +- Requiring the gem with the environment variable `DISABLE_DEAD_END_CORE_EXT=1` now disables monkeypatching core extensions (https://github.com/zombocom/dead_end/pull/119) + ## 3.0.3 - Expand explanations coming from additional Ripper errors (https://github.com/zombocom/dead_end/pull/117) diff --git a/README.md b/README.md index 9382b44..fc03a73 100644 --- a/README.md +++ b/README.md @@ -166,6 +166,16 @@ Here's an example: ![](assets/syntax_search.gif) +## Use internals + +To use the `dead_end` gem without monkeypatching you can set the environment variable `DISABLE_DEAD_END_CORE_EXT=1` before requiring the gem. This will allow you to load `dead_end` and use its internals without mutating `require`. + +Stable internal interface(s): + +- `DeadEnd.handle_error(e)` + +Any other entrypoints are subject to change without warning. If you want to use an internal interface from `dead_end` not on this list, open an issue to explain your use case. + ## Development After checking out the repo, run `bin/setup` to install dependencies. Then, run `rake spec` to run the tests. You can also run `bin/console` for an interactive prompt that will allow you to experiment. diff --git a/lib/dead_end.rb b/lib/dead_end.rb index fcbcb8b..04cf69f 100644 --- a/lib/dead_end.rb +++ b/lib/dead_end.rb @@ -16,18 +16,41 @@ module DeadEnd class Error < StandardError; end TIMEOUT_DEFAULT = ENV.fetch("DEAD_END_TIMEOUT", 1).to_i - def self.handle_error(e) + # DeadEnd.handle_error [Public interface] + # + # Takes an exception from a syntax error, uses that + # error message to locate the file. Then the file + # will be analyzed to find the location of the syntax + # error and emit that location to stderr. + # + # Example: + # + # begin + # require 'bad_file' + # rescue => e + # DeadEnd.handle_error(e) + # end + # + # By default it will re_raise the exception unless + # `re_raise: false`. The message output location + # can be configured using the `io: $stderr` input. + # + # If a valid filename cannot be determined, the original + # exception will be re-raised (even with + # `re_raise: false`). + def self.handle_error(e, re_raise: true, io: $stderr) file = PathnameFromMessage.new(e.message).call.name raise e unless file - $stderr.sync = true + io.sync = true call( + io: io, source: file.read, filename: file ) - raise e + raise e if re_raise end def self.record_dir(dir) @@ -68,6 +91,8 @@ def self.indent(string) end end + # DeadEnd.valid_without? [Private interface] + # # This will tell you if the `code_lines` would be valid # if you removed the `without_lines`. In short it's a # way to detect if we've found the lines with syntax errors @@ -102,6 +127,8 @@ def self.invalid?(source) Ripper.new(source).tap(&:parse).error? end + # DeadEnd.valid? [Private interface] + # # Returns truthy if a given input source is valid syntax # # DeadEnd.valid?(<<~EOM) # => true @@ -143,7 +170,7 @@ def self.valid?(source) # Integration require_relative "dead_end/cli" -require_relative "dead_end/auto" +require_relative "dead_end/core_ext" unless ENV["DISABLE_DEAD_END_CORE_EXT"] # Core logic require_relative "dead_end/code_search" diff --git a/lib/dead_end/auto.rb b/lib/dead_end/auto.rb index fe0336e..c96fe2d 100644 --- a/lib/dead_end/auto.rb +++ b/lib/dead_end/auto.rb @@ -1,35 +1,6 @@ # frozen_string_literal: true require_relative "../dead_end" +require_relative "core_ext" -# Monkey patch kernel to ensure that all `require` calls call the same -# method -module Kernel - module_function - - alias_method :dead_end_original_require, :require - alias_method :dead_end_original_require_relative, :require_relative - alias_method :dead_end_original_load, :load - - def load(file, wrap = false) - dead_end_original_load(file) - rescue SyntaxError => e - DeadEnd.handle_error(e) - end - - def require(file) - dead_end_original_require(file) - rescue SyntaxError => e - DeadEnd.handle_error(e) - end - - def require_relative(file) - if Pathname.new(file).absolute? - dead_end_original_require file - else - dead_end_original_require File.expand_path("../#{file}", Kernel.caller_locations(1, 1)[0].absolute_path) - end - rescue SyntaxError => e - DeadEnd.handle_error(e) - end -end +warn "Calling `require 'dead_end/auto'` is deprecated, please `require 'dead_end'` instead." diff --git a/lib/dead_end/core_ext.rb b/lib/dead_end/core_ext.rb new file mode 100644 index 0000000..c89abd5 --- /dev/null +++ b/lib/dead_end/core_ext.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +# Monkey patch kernel to ensure that all `require` calls call the same +# method +module Kernel + module_function + + alias_method :dead_end_original_require, :require + alias_method :dead_end_original_require_relative, :require_relative + alias_method :dead_end_original_load, :load + + def load(file, wrap = false) + dead_end_original_load(file) + rescue SyntaxError => e + DeadEnd.handle_error(e) + end + + def require(file) + dead_end_original_require(file) + rescue SyntaxError => e + DeadEnd.handle_error(e) + end + + def require_relative(file) + if Pathname.new(file).absolute? + dead_end_original_require file + else + dead_end_original_require File.expand_path("../#{file}", Kernel.caller_locations(1, 1)[0].absolute_path) + end + rescue SyntaxError => e + DeadEnd.handle_error(e) + end +end diff --git a/spec/integration/dead_end_spec.rb b/spec/integration/dead_end_spec.rb index dc093bb..68c1cc6 100644 --- a/spec/integration/dead_end_spec.rb +++ b/spec/integration/dead_end_spec.rb @@ -5,6 +5,22 @@ module DeadEnd RSpec.describe "Integration tests that don't spawn a process (like using the cli)" do + it "has a `handle_error` interface" do + fake_error = Object.new + def fake_error.message + "#{__FILE__}:216: unterminated string meets end of file " + end + + io = StringIO.new + DeadEnd.handle_error( + fake_error, + re_raise: false, + io: io + ) + + expect(io.string.strip).to eq("Syntax OK") + end + it "returns good results on routes.rb" do source = fixtures_dir.join("routes.rb.txt").read diff --git a/spec/integration/ruby_command_line_spec.rb b/spec/integration/ruby_command_line_spec.rb index 9cd7e15..f167f43 100644 --- a/spec/integration/ruby_command_line_spec.rb +++ b/spec/integration/ruby_command_line_spec.rb @@ -53,6 +53,12 @@ module DeadEnd expect($?.success?).to be_falsey expect(out).to include('❯ 5 it "flerg"').once + + # Does not monkeypatch when env var is set + out = `DISABLE_DEAD_END_CORE_EXT=1 ruby -I#{lib_dir} -rdead_end #{require_rb} 2>&1` + + expect($?.success?).to be_falsey + expect(out).to_not include('❯ 5 it "flerg"').once end end end From dc18d1a32b04635519bd225e31db49e7c6cd35ae Mon Sep 17 00:00:00 2001 From: schneems Date: Fri, 19 Nov 2021 15:05:15 -0600 Subject: [PATCH 2/4] Move api to `dead_end/api` instead of env var Because setting environment variables on systems may be difficult or inconsistent we are moving the API to be require based. Now anyone who wants to use dead_end without mutating `require` can do so via requiring `dead_end/api`. --- CHANGELOG.md | 2 +- README.md | 2 +- lib/dead_end.rb | 191 +-------------------- lib/dead_end/api.rb | 191 +++++++++++++++++++++ spec/integration/dead_end_spec.rb | 16 -- spec/integration/ruby_command_line_spec.rb | 17 +- spec/unit/api_spec.rb | 24 +++ 7 files changed, 229 insertions(+), 214 deletions(-) create mode 100644 lib/dead_end/api.rb create mode 100644 spec/unit/api_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 14d574b..ac88977 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,8 +1,8 @@ ## HEAD (unreleased) - Requiring `dead_end/auto` is now deprecated please require `dead_end` instead (https://github.com/zombocom/dead_end/pull/119) +- Requiring `dead_end/api` now loads code without monkeypatching core extensions (https://github.com/zombocom/dead_end/pull/119) - The interface `DeadEnd.handle_error` is declared public and stable (https://github.com/zombocom/dead_end/pull/119) -- Requiring the gem with the environment variable `DISABLE_DEAD_END_CORE_EXT=1` now disables monkeypatching core extensions (https://github.com/zombocom/dead_end/pull/119) ## 3.0.3 diff --git a/README.md b/README.md index fc03a73..f74df09 100644 --- a/README.md +++ b/README.md @@ -168,7 +168,7 @@ Here's an example: ## Use internals -To use the `dead_end` gem without monkeypatching you can set the environment variable `DISABLE_DEAD_END_CORE_EXT=1` before requiring the gem. This will allow you to load `dead_end` and use its internals without mutating `require`. +To use the `dead_end` gem without monkeypatching you can `require 'dead_en/api'`. This will allow you to load `dead_end` and use its internals without mutating `require`. Stable internal interface(s): diff --git a/lib/dead_end.rb b/lib/dead_end.rb index 04cf69f..9b98b97 100644 --- a/lib/dead_end.rb +++ b/lib/dead_end.rb @@ -1,191 +1,4 @@ # frozen_string_literal: true -require_relative "dead_end/version" - -require "tmpdir" -require "stringio" -require "pathname" -require "ripper" -require "timeout" - -module DeadEnd - # Used to indicate a default value that cannot - # be confused with another input - DEFAULT_VALUE = Object.new.freeze - - class Error < StandardError; end - TIMEOUT_DEFAULT = ENV.fetch("DEAD_END_TIMEOUT", 1).to_i - - # DeadEnd.handle_error [Public interface] - # - # Takes an exception from a syntax error, uses that - # error message to locate the file. Then the file - # will be analyzed to find the location of the syntax - # error and emit that location to stderr. - # - # Example: - # - # begin - # require 'bad_file' - # rescue => e - # DeadEnd.handle_error(e) - # end - # - # By default it will re_raise the exception unless - # `re_raise: false`. The message output location - # can be configured using the `io: $stderr` input. - # - # If a valid filename cannot be determined, the original - # exception will be re-raised (even with - # `re_raise: false`). - def self.handle_error(e, re_raise: true, io: $stderr) - file = PathnameFromMessage.new(e.message).call.name - raise e unless file - - io.sync = true - - call( - io: io, - source: file.read, - filename: file - ) - - raise e if re_raise - end - - def self.record_dir(dir) - time = Time.now.strftime("%Y-%m-%d-%H-%M-%s-%N") - dir = Pathname(dir) - symlink = dir.join("last").tap { |path| path.delete if path.exist? } - dir.join(time).tap { |path| - path.mkpath - FileUtils.symlink(path.basename, symlink) - } - end - - def self.call(source:, filename: DEFAULT_VALUE, terminal: DEFAULT_VALUE, record_dir: nil, timeout: TIMEOUT_DEFAULT, io: $stderr) - search = nil - filename = nil if filename == DEFAULT_VALUE - Timeout.timeout(timeout) do - record_dir ||= ENV["DEBUG"] ? "tmp" : nil - search = CodeSearch.new(source, record_dir: record_dir).call - end - - blocks = search.invalid_blocks - DisplayInvalidBlocks.new( - io: io, - blocks: blocks, - filename: filename, - terminal: terminal, - code_lines: search.code_lines - ).call - rescue Timeout::Error => e - io.puts "Search timed out DEAD_END_TIMEOUT=#{timeout}, run with DEBUG=1 for more info" - io.puts e.backtrace.first(3).join($/) - end - - # Used for counting spaces - module SpaceCount - def self.indent(string) - string.split(/\S/).first&.length || 0 - end - end - - # DeadEnd.valid_without? [Private interface] - # - # This will tell you if the `code_lines` would be valid - # if you removed the `without_lines`. In short it's a - # way to detect if we've found the lines with syntax errors - # in our document yet. - # - # code_lines = [ - # CodeLine.new(line: "def foo\n", index: 0) - # CodeLine.new(line: " def bar\n", index: 1) - # CodeLine.new(line: "end\n", index: 2) - # ] - # - # DeadEnd.valid_without?( - # without_lines: code_lines[1], - # code_lines: code_lines - # ) # => true - # - # DeadEnd.valid?(code_lines) # => false - def self.valid_without?(without_lines:, code_lines:) - lines = code_lines - Array(without_lines).flatten - - if lines.empty? - true - else - valid?(lines) - end - end - - def self.invalid?(source) - source = source.join if source.is_a?(Array) - source = source.to_s - - Ripper.new(source).tap(&:parse).error? - end - - # DeadEnd.valid? [Private interface] - # - # Returns truthy if a given input source is valid syntax - # - # DeadEnd.valid?(<<~EOM) # => true - # def foo - # end - # EOM - # - # DeadEnd.valid?(<<~EOM) # => false - # def foo - # def bar # Syntax error here - # end - # EOM - # - # You can also pass in an array of lines and they'll be - # joined before evaluating - # - # DeadEnd.valid?( - # [ - # "def foo\n", - # "end\n" - # ] - # ) # => true - # - # DeadEnd.valid?( - # [ - # "def foo\n", - # " def bar\n", # Syntax error here - # "end\n" - # ] - # ) # => false - # - # As an FYI the CodeLine class instances respond to `to_s` - # so passing a CodeLine in as an object or as an array - # will convert it to it's code representation. - def self.valid?(source) - !invalid?(source) - end -end - -# Integration -require_relative "dead_end/cli" -require_relative "dead_end/core_ext" unless ENV["DISABLE_DEAD_END_CORE_EXT"] - -# Core logic -require_relative "dead_end/code_search" -require_relative "dead_end/code_frontier" -require_relative "dead_end/explain_syntax" -require_relative "dead_end/clean_document" - -# Helpers -require_relative "dead_end/lex_all" -require_relative "dead_end/code_line" -require_relative "dead_end/code_block" -require_relative "dead_end/block_expand" -require_relative "dead_end/ripper_errors" -require_relative "dead_end/insertion_sort" -require_relative "dead_end/around_block_scan" -require_relative "dead_end/pathname_from_message" -require_relative "dead_end/display_invalid_blocks" -require_relative "dead_end/parse_blocks_from_indent_line" +require_relative "dead_end/api" +require_relative "dead_end/core_ext" diff --git a/lib/dead_end/api.rb b/lib/dead_end/api.rb new file mode 100644 index 0000000..6077477 --- /dev/null +++ b/lib/dead_end/api.rb @@ -0,0 +1,191 @@ +require_relative "version" + +require "tmpdir" +require "stringio" +require "pathname" +require "ripper" +require "timeout" + +module DeadEnd + # Used to indicate a default value that cannot + # be confused with another input. + DEFAULT_VALUE = Object.new.freeze + + class Error < StandardError; end + TIMEOUT_DEFAULT = ENV.fetch("DEAD_END_TIMEOUT", 1).to_i + + # DeadEnd.handle_error [Public] + # + # Takes an exception from a syntax error, uses that + # error message to locate the file. Then the file + # will be analyzed to find the location of the syntax + # error and emit that location to stderr. + # + # Example: + # + # begin + # require 'bad_file' + # rescue => e + # DeadEnd.handle_error(e) + # end + # + # By default it will re_raise the exception unless + # `re_raise: false`. The message output location + # can be configured using the `io: $stderr` input. + # + # If a valid filename cannot be determined, the original + # exception will be re-raised (even with + # `re_raise: false`). + def self.handle_error(e, re_raise: true, io: $stderr) + file = PathnameFromMessage.new(e.message).call.name + raise e unless file + + io.sync = true + + call( + io: io, + source: file.read, + filename: file + ) + + raise e if re_raise + end + + # DeadEnd.call [Private] + # + # Main private interface + def self.call(source:, filename: DEFAULT_VALUE, terminal: DEFAULT_VALUE, record_dir: nil, timeout: TIMEOUT_DEFAULT, io: $stderr) + search = nil + filename = nil if filename == DEFAULT_VALUE + Timeout.timeout(timeout) do + record_dir ||= ENV["DEBUG"] ? "tmp" : nil + search = CodeSearch.new(source, record_dir: record_dir).call + end + + blocks = search.invalid_blocks + DisplayInvalidBlocks.new( + io: io, + blocks: blocks, + filename: filename, + terminal: terminal, + code_lines: search.code_lines + ).call + rescue Timeout::Error => e + io.puts "Search timed out DEAD_END_TIMEOUT=#{timeout}, run with DEBUG=1 for more info" + io.puts e.backtrace.first(3).join($/) + end + + # DeadEnd.record_dir [Private] + # + # Used to generate a unique directory to record + # search steps for debugging + def self.record_dir(dir) + time = Time.now.strftime("%Y-%m-%d-%H-%M-%s-%N") + dir = Pathname(dir) + symlink = dir.join("last").tap { |path| path.delete if path.exist? } + dir.join(time).tap { |path| + path.mkpath + FileUtils.symlink(path.basename, symlink) + } + end + + # DeadEnd.valid_without? [Private] + # + # This will tell you if the `code_lines` would be valid + # if you removed the `without_lines`. In short it's a + # way to detect if we've found the lines with syntax errors + # in our document yet. + # + # code_lines = [ + # CodeLine.new(line: "def foo\n", index: 0) + # CodeLine.new(line: " def bar\n", index: 1) + # CodeLine.new(line: "end\n", index: 2) + # ] + # + # DeadEnd.valid_without?( + # without_lines: code_lines[1], + # code_lines: code_lines + # ) # => true + # + # DeadEnd.valid?(code_lines) # => false + def self.valid_without?(without_lines:, code_lines:) + lines = code_lines - Array(without_lines).flatten + + if lines.empty? + true + else + valid?(lines) + end + end + + # DeadEnd.invalid? [Private] + # + # Opposite of `DeadEnd.valid?` + def self.invalid?(source) + source = source.join if source.is_a?(Array) + source = source.to_s + + Ripper.new(source).tap(&:parse).error? + end + + # DeadEnd.valid? [Private] + # + # Returns truthy if a given input source is valid syntax + # + # DeadEnd.valid?(<<~EOM) # => true + # def foo + # end + # EOM + # + # DeadEnd.valid?(<<~EOM) # => false + # def foo + # def bar # Syntax error here + # end + # EOM + # + # You can also pass in an array of lines and they'll be + # joined before evaluating + # + # DeadEnd.valid?( + # [ + # "def foo\n", + # "end\n" + # ] + # ) # => true + # + # DeadEnd.valid?( + # [ + # "def foo\n", + # " def bar\n", # Syntax error here + # "end\n" + # ] + # ) # => false + # + # As an FYI the CodeLine class instances respond to `to_s` + # so passing a CodeLine in as an object or as an array + # will convert it to it's code representation. + def self.valid?(source) + !invalid?(source) + end +end + +# Integration +require_relative "cli" + +# Core logic +require_relative "code_search" +require_relative "code_frontier" +require_relative "explain_syntax" +require_relative "clean_document" + +# Helpers +require_relative "lex_all" +require_relative "code_line" +require_relative "code_block" +require_relative "block_expand" +require_relative "ripper_errors" +require_relative "insertion_sort" +require_relative "around_block_scan" +require_relative "pathname_from_message" +require_relative "display_invalid_blocks" +require_relative "parse_blocks_from_indent_line" diff --git a/spec/integration/dead_end_spec.rb b/spec/integration/dead_end_spec.rb index 68c1cc6..dc093bb 100644 --- a/spec/integration/dead_end_spec.rb +++ b/spec/integration/dead_end_spec.rb @@ -5,22 +5,6 @@ module DeadEnd RSpec.describe "Integration tests that don't spawn a process (like using the cli)" do - it "has a `handle_error` interface" do - fake_error = Object.new - def fake_error.message - "#{__FILE__}:216: unterminated string meets end of file " - end - - io = StringIO.new - DeadEnd.handle_error( - fake_error, - re_raise: false, - io: io - ) - - expect(io.string.strip).to eq("Syntax OK") - end - it "returns good results on routes.rb" do source = fixtures_dir.join("routes.rb.txt").read diff --git a/spec/integration/ruby_command_line_spec.rb b/spec/integration/ruby_command_line_spec.rb index f167f43..df82dca 100644 --- a/spec/integration/ruby_command_line_spec.rb +++ b/spec/integration/ruby_command_line_spec.rb @@ -11,19 +11,28 @@ module DeadEnd script.write <<~'EOM' puts Kernel.private_methods EOM + dead_end_methods_file = tmpdir.join("dead_end_methods.txt") + api_only_methods_file = tmpdir.join("api_only_methods.txt") kernel_methods_file = tmpdir.join("kernel_methods.txt") - d_pid = Process.spawn("ruby -I#{lib_dir} -rdead_end/auto #{script} 2>&1 > #{dead_end_methods_file}") + d_pid = Process.spawn("ruby -I#{lib_dir} -rdead_end #{script} 2>&1 > #{dead_end_methods_file}") k_pid = Process.spawn("ruby #{script} 2>&1 >> #{kernel_methods_file}") + r_pid = Process.spawn("ruby -I#{lib_dir} -rdead_end/api #{script} 2>&1 > #{api_only_methods_file}") Process.wait(k_pid) Process.wait(d_pid) + Process.wait(r_pid) dead_end_methods_array = dead_end_methods_file.read.strip.lines.map(&:strip) kernel_methods_array = kernel_methods_file.read.strip.lines.map(&:strip) + api_only_methods_array = api_only_methods_file.read.strip.lines.map(&:strip) + methods = (dead_end_methods_array - kernel_methods_array).sort expect(methods).to eq(["dead_end_original_load", "dead_end_original_require", "dead_end_original_require_relative", "timeout"]) + + methods = (api_only_methods_array - kernel_methods_array).sort + expect(methods).to eq(["timeout"]) end end @@ -53,12 +62,6 @@ module DeadEnd expect($?.success?).to be_falsey expect(out).to include('❯ 5 it "flerg"').once - - # Does not monkeypatch when env var is set - out = `DISABLE_DEAD_END_CORE_EXT=1 ruby -I#{lib_dir} -rdead_end #{require_rb} 2>&1` - - expect($?.success?).to be_falsey - expect(out).to_not include('❯ 5 it "flerg"').once end end end diff --git a/spec/unit/api_spec.rb b/spec/unit/api_spec.rb new file mode 100644 index 0000000..de8f746 --- /dev/null +++ b/spec/unit/api_spec.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +require_relative "../spec_helper" +require "ruby-prof" + +module DeadEnd + RSpec.describe "Top level DeadEnd api" do + it "has a `handle_error` interface" do + fake_error = Object.new + def fake_error.message + "#{__FILE__}:216: unterminated string meets end of file " + end + + io = StringIO.new + DeadEnd.handle_error( + fake_error, + re_raise: false, + io: io + ) + + expect(io.string.strip).to eq("Syntax OK") + end + end +end From 78297ee64c10de0819d8d131c93af6d6055b9106 Mon Sep 17 00:00:00 2001 From: Richard Schneeman Date: Sat, 20 Nov 2021 15:16:22 -0600 Subject: [PATCH 3/4] Update lib/dead_end/api.rb Co-authored-by: Jake Zimmerman --- lib/dead_end/api.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/dead_end/api.rb b/lib/dead_end/api.rb index 6077477..8078b20 100644 --- a/lib/dead_end/api.rb +++ b/lib/dead_end/api.rb @@ -29,7 +29,7 @@ class Error < StandardError; end # DeadEnd.handle_error(e) # end # - # By default it will re_raise the exception unless + # By default it will re-raise the exception unless # `re_raise: false`. The message output location # can be configured using the `io: $stderr` input. # From 0b407461b9bf985d61955eb6addc723a3ca3968c Mon Sep 17 00:00:00 2001 From: schneems Date: Sat, 20 Nov 2021 15:58:38 -0600 Subject: [PATCH 4/4] Add explicit check for Syntax Error As suggested in https://github.com/zombocom/dead_end/pull/119#discussion_r753592117 we are now explicitly checking the type of the incoming error. Rather than raise a new error, I've chosen to emit a warning and re-raise the original error. This is a similar pattern to the case where we are not able to detect the filename from the error message. The goal is to provide visibility into the source of the behavior, but to break as few expectations as possible. The `DeadEnd.handle_error` should (ideally) never raise an error to the end user. The purpose of the library is to provide progressive error enhancement. If it fails it means we cannot make a better error, so therefore we should warn (so the user can fix or report the problem) and then fall back to the original error. I also added a test for the failure error condition where the filename cannot be pulled out of the SyntaxError message which was added in https://github.com/zombocom/dead_end/pull/114. --- lib/dead_end/api.rb | 9 +++++-- lib/dead_end/pathname_from_message.rb | 2 +- spec/unit/api_spec.rb | 38 +++++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 3 deletions(-) diff --git a/lib/dead_end/api.rb b/lib/dead_end/api.rb index 8078b20..04edf07 100644 --- a/lib/dead_end/api.rb +++ b/lib/dead_end/api.rb @@ -16,7 +16,7 @@ class Error < StandardError; end # DeadEnd.handle_error [Public] # - # Takes an exception from a syntax error, uses that + # Takes a `SyntaxError`` exception, uses the # error message to locate the file. Then the file # will be analyzed to find the location of the syntax # error and emit that location to stderr. @@ -37,7 +37,12 @@ class Error < StandardError; end # exception will be re-raised (even with # `re_raise: false`). def self.handle_error(e, re_raise: true, io: $stderr) - file = PathnameFromMessage.new(e.message).call.name + unless e.is_a?(SyntaxError) + io.puts("DeadEnd: Must pass a SyntaxError, got: #{e.class}") + raise e + end + + file = PathnameFromMessage.new(e.message, io: io).call.name raise e unless file io.sync = true diff --git a/lib/dead_end/pathname_from_message.rb b/lib/dead_end/pathname_from_message.rb index e3141eb..1ee9f53 100644 --- a/lib/dead_end/pathname_from_message.rb +++ b/lib/dead_end/pathname_from_message.rb @@ -30,7 +30,7 @@ def call end if @parts.empty? - @io.puts "DeadEnd: could not find filename from #{@line.inspect}" + @io.puts "DeadEnd: Could not find filename from #{@line.inspect}" @name = nil end diff --git a/spec/unit/api_spec.rb b/spec/unit/api_spec.rb index de8f746..e714b58 100644 --- a/spec/unit/api_spec.rb +++ b/spec/unit/api_spec.rb @@ -11,6 +11,10 @@ def fake_error.message "#{__FILE__}:216: unterminated string meets end of file " end + def fake_error.is_a?(v) + true + end + io = StringIO.new DeadEnd.handle_error( fake_error, @@ -20,5 +24,39 @@ def fake_error.message expect(io.string.strip).to eq("Syntax OK") end + + it "raises original error with warning if a non-syntax error is passed" do + error = NameError.new("blerg") + io = StringIO.new + expect { + DeadEnd.handle_error( + error, + re_raise: false, + io: io + ) + }.to raise_error { |e| + expect(io.string).to include("Must pass a SyntaxError") + expect(e).to eq(error) + } + end + + it "raises original error with warning if file is not found" do + fake_error = SyntaxError.new + def fake_error.message + "#does/not/exist/lol/doesnotexist:216: unterminated string meets end of file " + end + + io = StringIO.new + expect { + DeadEnd.handle_error( + fake_error, + re_raise: false, + io: io + ) + }.to raise_error { |e| + expect(io.string).to include("Could not find filename") + expect(e).to eq(fake_error) + } + end end end