-
Notifications
You must be signed in to change notification settings - Fork 63
Add opt-in binary string diff support #301
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -0,0 +1,28 @@ | ||||
| # frozen_string_literal: true | ||||
|
|
||||
| require 'super_diff/binary_string/differs' | ||||
| require 'super_diff/binary_string/inspection_tree_builders' | ||||
| require 'super_diff/binary_string/operation_trees' | ||||
| require 'super_diff/binary_string/operation_tree_builders' | ||||
| require 'super_diff/binary_string/operation_tree_flatteners' | ||||
|
|
||||
| module SuperDiff | ||||
| module BinaryString | ||||
| def self.applies_to?(*values) | ||||
| values.all? { |value| value.is_a?(::String) && value.encoding == Encoding::ASCII_8BIT } | ||||
| end | ||||
|
|
||||
| SuperDiff.configure do |config| | ||||
| config.prepend_extra_differ_classes(Differs::BinaryString) | ||||
| config.prepend_extra_operation_tree_builder_classes( | ||||
| OperationTreeBuilders::BinaryString | ||||
| ) | ||||
| config.prepend_extra_operation_tree_classes( | ||||
| OperationTrees::BinaryString | ||||
| ) | ||||
|
Comment on lines
+20
to
+22
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What effect will this have on the super_diff/lib/super_diff/rspec/operation_tree_builders/object_having_attributes.rb Line 14 in 25ceb5f
Can you add a test for this, or drop it if it has an adverse effect? |
||||
| config.prepend_extra_inspection_tree_builder_classes( | ||||
| InspectionTreeBuilders::BinaryString | ||||
| ) | ||||
|
Comment on lines
+23
to
+25
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By the way, in contrast to the above, let's keep the binary string inspection tree builder – it makes the output a lot cleaner for this extension's use case: |
||||
| end | ||||
| end | ||||
| end | ||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module SuperDiff | ||
| module BinaryString | ||
| module Differs | ||
| autoload( | ||
| :BinaryString, | ||
| 'super_diff/binary_string/differs/binary_string' | ||
| ) | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module SuperDiff | ||
| module BinaryString | ||
| module Differs | ||
| class BinaryString < Core::AbstractDiffer | ||
| def self.applies_to?(expected, actual) | ||
| SuperDiff::BinaryString.applies_to?(expected, actual) | ||
| end | ||
|
|
||
| protected | ||
|
|
||
| def operation_tree_builder_class | ||
| OperationTreeBuilders::BinaryString | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module SuperDiff | ||
| module BinaryString | ||
| module InspectionTreeBuilders | ||
| autoload( | ||
| :BinaryString, | ||
| 'super_diff/binary_string/inspection_tree_builders/binary_string' | ||
| ) | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module SuperDiff | ||
| module BinaryString | ||
| module InspectionTreeBuilders | ||
| class BinaryString < Core::AbstractInspectionTreeBuilder | ||
| def self.applies_to?(value) | ||
| SuperDiff::BinaryString.applies_to?(value) | ||
| end | ||
|
|
||
| def call | ||
| Core::InspectionTree.new do |t| | ||
| t.add_text "<binary string (#{object.bytesize} bytes)>" | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module SuperDiff | ||
| module BinaryString | ||
| module OperationTreeBuilders | ||
| autoload( | ||
| :BinaryString, | ||
| 'super_diff/binary_string/operation_tree_builders/binary_string' | ||
| ) | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module SuperDiff | ||
| module BinaryString | ||
| module OperationTreeBuilders | ||
| class BinaryString < Basic::OperationTreeBuilders::MultilineString | ||
| BYTES_PER_LINE = 16 | ||
| private_constant :BYTES_PER_LINE | ||
|
|
||
| def self.applies_to?(expected, actual) | ||
| SuperDiff::BinaryString.applies_to?(expected, actual) | ||
| end | ||
|
|
||
| def initialize(*args) | ||
| args.first[:expected] = binary_to_hex(args.first[:expected]) | ||
| args.first[:actual] = binary_to_hex(args.first[:actual]) | ||
|
|
||
| super | ||
| end | ||
|
|
||
| protected | ||
|
|
||
| def build_operation_tree | ||
| OperationTrees::BinaryString.new([]) | ||
| end | ||
|
|
||
| # Prevent creation of BinaryOperation objects which the flattener | ||
| # cannot handle | ||
| def should_compare?(_operation, _next_operation) | ||
| false | ||
| end | ||
|
Comment on lines
+29
to
+31
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting. I ended up going down a rabbit hole here. In practice, there are currently no plain string operation tree builders / differs, so this should never happen. However, in theory, this is correct: if there were any operation tree builders that applied to two non-binary-encoded1 strings, the I'd normally ask for a test for this case, but since the same is true for the EDIT: #304 is now in main, feel free to merge and get rid of this method. Footnotes
|
||
|
|
||
| private | ||
|
|
||
| def split_into_lines(string) | ||
| super.map { |line| line.delete_suffix("\n") }.reject(&:empty?) | ||
| end | ||
|
|
||
| def binary_to_hex(data) | ||
| data | ||
| .each_byte | ||
| .each_slice(BYTES_PER_LINE) | ||
| .with_index | ||
| .map { |bytes, index| format_hex_line(index * BYTES_PER_LINE, bytes) } | ||
| .join("\n") | ||
| end | ||
|
|
||
| def format_hex_line(offset, bytes) | ||
| hex_pairs = bytes | ||
| .map { |b| format('%02x', b) } | ||
| .each_slice(2) | ||
| .map(&:join) | ||
| .join(' ') | ||
|
|
||
| ascii = bytes.map { |b| printable_char(b) }.join | ||
|
|
||
| format('%<offset>08x: %<hex>-39s %<ascii>s', offset:, hex: hex_pairs, ascii:) | ||
| end | ||
|
|
||
| def printable_char(byte) | ||
| byte >= 32 && byte < 127 ? byte.chr : '.' | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module SuperDiff | ||
| module BinaryString | ||
| module OperationTreeFlatteners | ||
| autoload( | ||
| :BinaryString, | ||
| 'super_diff/binary_string/operation_tree_flatteners/binary_string' | ||
| ) | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module SuperDiff | ||
| module BinaryString | ||
| module OperationTreeFlatteners | ||
| class BinaryString < Core::AbstractOperationTreeFlattener | ||
| def build_tiered_lines | ||
| operation_tree.map do |operation| | ||
| Core::Line.new( | ||
| type: operation.name, | ||
| indentation_level: indentation_level, | ||
| value: operation.value | ||
| ) | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module SuperDiff | ||
| module BinaryString | ||
| module OperationTrees | ||
| autoload( | ||
| :BinaryString, | ||
| 'super_diff/binary_string/operation_trees/binary_string' | ||
| ) | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module SuperDiff | ||
| module BinaryString | ||
| module OperationTrees | ||
| class BinaryString < Core::AbstractOperationTree | ||
| def self.applies_to?(value) | ||
| SuperDiff::BinaryString.applies_to?(value) | ||
| end | ||
|
|
||
| protected | ||
|
|
||
| def operation_tree_flattener_class | ||
| OperationTreeFlatteners::BinaryString | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,87 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require 'spec_helper' | ||
| require 'super_diff/binary_string' | ||
|
|
||
| RSpec.describe 'Integration with binary strings', type: :integration do | ||
| context 'when comparing two different binary strings' do | ||
| it 'produces the correct failure message' do | ||
| as_both_colored_and_uncolored do |color_enabled| | ||
| snippet = <<~TEST.strip | ||
| require 'super_diff/binary_string' | ||
| actual = "Hello".b | ||
| expected = "World".b | ||
| expect(actual).to eq(expected) | ||
| TEST | ||
| program = | ||
| make_plain_test_program(snippet, color_enabled: color_enabled) | ||
|
|
||
| expected_output = | ||
| build_expected_output( | ||
| color_enabled: color_enabled, | ||
| snippet: 'expect(actual).to eq(expected)', | ||
| expectation: | ||
| proc do | ||
| line do | ||
| plain 'Expected ' | ||
| actual '<binary string (5 bytes)>' | ||
| plain ' to eq ' | ||
| expected '<binary string (5 bytes)>' | ||
| plain '.' | ||
| end | ||
| end, | ||
| diff: | ||
| proc do | ||
| expected_line '- 00000000: 576f 726c 64 World' | ||
| actual_line '+ 00000000: 4865 6c6c 6f Hello' | ||
| end | ||
| ) | ||
|
|
||
| expect(program).to produce_output_when_run(expected_output).in_color( | ||
| color_enabled | ||
| ) | ||
| end | ||
| end | ||
| end | ||
|
|
||
| context 'when comparing binary strings spanning multiple lines' do | ||
| it 'produces a multi-line hex dump diff' do | ||
| as_both_colored_and_uncolored do |color_enabled| | ||
| snippet = <<~TEST.strip | ||
| require 'super_diff/binary_string' | ||
| actual = ("A" * 20).b | ||
| expected = ("A" * 16 + "B" * 4).b | ||
| expect(actual).to eq(expected) | ||
| TEST | ||
| program = | ||
| make_plain_test_program(snippet, color_enabled: color_enabled) | ||
|
|
||
| expected_output = | ||
| build_expected_output( | ||
| color_enabled: color_enabled, | ||
| snippet: 'expect(actual).to eq(expected)', | ||
| expectation: | ||
| proc do | ||
| line do | ||
| plain 'Expected ' | ||
| actual '<binary string (20 bytes)>' | ||
| plain ' to eq ' | ||
| expected '<binary string (20 bytes)>' | ||
| plain '.' | ||
| end | ||
| end, | ||
| diff: | ||
| proc do | ||
| plain_line ' 00000000: 4141 4141 4141 4141 4141 4141 4141 4141 AAAAAAAAAAAAAAAA' | ||
| expected_line '- 00000010: 4242 4242 BBBB' | ||
| actual_line '+ 00000010: 4141 4141 AAAA' | ||
| end | ||
| ) | ||
|
|
||
| expect(program).to produce_output_when_run(expected_output).in_color( | ||
| color_enabled | ||
| ) | ||
| end | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This call makes the operation tree builder available to the entire diffing system, which makes for some funky output when diffing binary strings in collections:
So, let's remove this. The
Differs::BinaryStringwill still be available to diff top-level binary strings, and it will still use the BinaryString op tree builder.