This repository has been archived by the owner on Nov 30, 2024. It is now read-only.
-
-
Notifications
You must be signed in to change notification settings - Fork 103
Fixes diff when fuzzy finder anything
is used in a Hash object (proof of concept)
#596
Closed
KarlHeitmann
wants to merge
11
commits into
rspec:main
from
KarlHeitmann:fix_diff_fuzzy_matcher_anything
Closed
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
8c81146
fixes diff when fuzzy finder anything is used
KarlHeitmann e4ece3d
fix works in hashes with nested hashes
KarlHeitmann 4de7d8c
fix: gets rid of Metrics::CyclomaticComplexity in Differ#diff
KarlHeitmann d50d742
fix: gets rid of Metrics::PerceivedComplexity in Diff#diff
KarlHeitmann 41e3c21
fix: gets rid of Metrics/MethodLength in Diff#diff
KarlHeitmann 15627b7
fix: error build ruby 2.4,3,2: complain in prepend
KarlHeitmann f9a9e9d
fix: rearrange test to pass legacy ruby build 1.8.7 & REE
KarlHeitmann d44a555
fix: short circuit recursive_get_key when user opts out Mocks
KarlHeitmann 2bb81a4
refactor: changes instance var by a returned local var
KarlHeitmann 8be1fd8
refactor: marks tests for fuzzy matcher broken on 1.8.7
KarlHeitmann 47cf1b2
adds private method Differ#run to get rid of PerceivedComplexity offense
KarlHeitmann File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,12 +13,14 @@ class Differ | |
def diff(actual, expected) | ||
diff = "" | ||
|
||
no_procs_no_numbers = lambda {|var1, var2| no_procs?(var1, var2) && no_numbers?(var1, var2)} | ||
|
||
unless actual.nil? || expected.nil? | ||
if all_strings?(actual, expected) | ||
if any_multiline_strings?(actual, expected) | ||
diff = diff_as_string(coerce_to_string(actual), coerce_to_string(expected)) | ||
end | ||
elsif no_procs?(actual, expected) && no_numbers?(actual, expected) | ||
diff = diff_as_string(coerce_to_string(actual), coerce_to_string(expected)) if any_multiline_strings?(actual, expected) | ||
elsif hash_with_anything?(expected) | ||
diff = diff_as_object_with_anything(actual, expected) if no_procs_no_numbers.call(actual, expected) | ||
elsif no_procs_no_numbers.call(actual, expected) | ||
diff = diff_as_object(actual, expected) | ||
end | ||
end | ||
|
@@ -56,6 +58,20 @@ def diff_as_string(actual, expected) | |
end | ||
# rubocop:enable Metrics/MethodLength | ||
|
||
def diff_as_object_with_anything(actual, expected) | ||
@keys_with_anything.each do |keys| | ||
pointer_expected = expected | ||
pointer_actual = actual | ||
final_key = keys.pop | ||
keys.each do |k| | ||
pointer_expected = pointer_expected[k] | ||
pointer_actual = pointer_actual[k] | ||
end | ||
pointer_expected[final_key] = pointer_actual[final_key] | ||
end | ||
diff_as_object(actual, expected) | ||
end | ||
|
||
def diff_as_object(actual, expected) | ||
actual_as_string = object_to_string(actual) | ||
expected_as_string = object_to_string(expected) | ||
|
@@ -73,6 +89,31 @@ def initialize(opts={}) | |
|
||
private | ||
|
||
def hash_with_anything?(arg) | ||
return false unless Hash === arg | ||
|
||
@keys_with_anything = recursive_get_keys(arg) | ||
pirj marked this conversation as resolved.
Show resolved
Hide resolved
|
||
@keys_with_anything.any? | ||
end | ||
|
||
def recursive_get_keys(hash) | ||
klass = RSpec::Mocks::ArgumentMatchers::AnyArgMatcher | ||
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 if this is not defined, like someone opted out of mocking completely, or uses rr/mocha etc? 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. I think it’s ok to use ‘return [] unless defined?(RSpec::Mocks)’ here 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. Nice! I didn't thought someone may opt out mocking completely. I've fixed it on my latest commit! |
||
hash.reduce([]) do |acc, pair| | ||
if klass === pair[1] | ||
acc << [pair[0]] | ||
else | ||
if Hash === pair[1] | ||
keys = recursive_get_keys(pair[1]) | ||
keys.each do |key| | ||
key.unshift pair[0] | ||
acc << key | ||
end | ||
end | ||
end | ||
acc | ||
end | ||
end | ||
|
||
def no_procs?(*args) | ||
safely_flatten(args).none? { |a| Proc === a } | ||
end | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
To reset the discussion below.
I am concerned about side effects of shared state in instance variables.
I’d live to get rid of an instance variable
@keys_with_anything
.Only those two methods need it. Can we cache it somehow here in a regular variable? Can
hash_with_anything?
become something that returns a value that we can subsequently pass todiff_as_object_with_anything
?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.
Ok! I just came up with something, I didn't thought previously about shared state. Now, I've renamed
hash_with_anything?
toall_keys_from_hash
and it returns akeys
array, that is later used indiff_as_object_with_anything
. This way we don't have shared state in instance variables.This approach involved an assignment inside an if condition, it added an extra
&&
operator and it increased the rubocop metric:PerceivedComplexity
score, so I had to create another method to wrap all the logic inside it.