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.

Sign up for GitHub

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

Jump to bottom

convert query values to strings before comparison #517

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

convert query values to strings before comparison #517

wants to merge 5 commits into from

Conversation

davidbegin
Copy link
Collaborator

No description provided.

@davidbegin davidbegin force-pushed the convert-values-to-strings-before-comparing branch 4 times, most recently from b1cb946 to 6e334ae Compare September 8, 2015 04:12
@davidbegin davidbegin mentioned this pull request Sep 8, 2015
Open
@davidbegin
Copy link
Collaborator Author

Some things I am still working on/figuring out:

  • figure out it this is the best place for this conversion
  • figure out if we should limit the classes we call .to_s on in the conversion
  • try this out with hash_including

@davidbegin davidbegin force-pushed the convert-values-to-strings-before-comparing branch from fa4583c to 5287372 Compare September 8, 2015 18:57
@davidbegin
Copy link
Collaborator Author

@Fire-Dragon-DoL you want to take a look and see if this fixes your problem, or add any other feedback?

@Fire-Dragon-DoL
Copy link

@presidentJFK I can't test against my code because it doesn't exist anymore (we changed it a lot), but by reviewing your PR it definitely looks like it fixes it entirely.

@davidbegin davidbegin force-pushed the convert-values-to-strings-before-comparing branch from c879f2e to 8d30ccf Compare September 13, 2015 05:20
@davidbegin
Copy link
Collaborator Author

It looks like this doesn't solve your original problem with hash_including, so I am giving this another look.

@Fire-Dragon-DoL
Copy link

Mhhh that's weird, handling the hash as string-values only on "both sides" of the hash_including, should work

@davidbegin
convert query values to strings before comparison
9510402
@davidbegin
spec for converting keys to symbols when Mocking
0eef3cd
@davidbegin
Refactor out .convert_query_string_into_array
07b906f 
   this commit refactors out a new private class method
  .convert_query_string_into_array from the .values_to_query method
  in WebMock::Util::QueryMapper
@davidbegin
clean up some private QueryMapper internals
0678821 
  rename private class method
  convert_query_string_into_array => .convert_query_hash_to_array
@davidbegin davidbegin force-pushed the convert-values-to-strings-before-comparing branch 3 times, most recently from 3caeea8 to 9fb40fc Compare September 17, 2015 08:27
@davidbegin
Converts to string for comparing w/ hash_including
08a3a4a 
  This change updates so the expected request and actual request,
  are compared to as strings, when using hash_including
@davidbegin davidbegin force-pushed the convert-values-to-strings-before-comparing branch from 9fb40fc to 08a3a4a Compare September 17, 2015 08:38
@davidbegin
Copy link
Collaborator Author

I made a couple mistakes when trying to fix this. I thought a simple one time stringify in the QueryMapper would fix the original issue.

But the problem with my original fix was it didn't take into account the HashIncludingMatcher. which handles its own comparision with #==, and was not converting the values to strings before comparing.

I have added the first spec you originally posted to the test suite, which exercises this exact functionality.
And updated the code to stringify before comparing values, fixing your test.

I now just have to clean up some changes I made, move specs to the right place, and double check
a couple more assumptions I have made, and we can get this merged in. And actually solve your original problem!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@davidbegin @Fire-Dragon-DoL