getsentry / responses

A utility for mocking out the Python Requests library.
Apache License 2.0
4.17k stars 356 forks source link

fix(matchers): Don't sort failed matches when printing error message #711

Closed mgaligniana closed 6 months ago

mgaligniana commented 7 months ago

Hi!

This PR removes the _create_key_val_str and with it the sorting when displaying matchers errors

Let me know any change required!

Thank you

Fixes GH-704

beliaev-maksim commented 7 months ago

@markstory I think we introduced _create_key_val_str for the compatibility between python2 and python3

should we just drop it completely and just present JSON?

beliaev-maksim commented 7 months ago

@mgaligniana please add changelog file entry

mgaligniana commented 7 months ago

@beliaev-maksim thank you for your fast response! On it! :factory_worker:

mgaligniana commented 7 months ago

@beliaev-maksim Done! Let me know if message is OK (I'm not English native speaker). Feel free to suggest changes! Thank you!

markstory commented 7 months ago

I think we introduced _create_key_val_str for the compatibility between python2 and python3 should we just drop it completely and just present JSON?

I think you're right on the origin of that method. We could likely drop it now that we're not supporting python 2 anymore.

mgaligniana commented 7 months ago

@markstory Do you want me to tackle in this pull request? Or in another ticket?

beliaev-maksim commented 7 months ago

@mgaligniana yes please

markstory commented 7 months ago

Do you want me to tackle in this pull request? Or in another ticket?

Using this pull request would be simpler for us.

mgaligniana commented 7 months ago

Done!

I had to update all test cases because now the output is not transformed to a whole string:

For example in query_param_matcher Before: ...doesn't match {array: [B, A, C]} After: ...doesn't match: {'array': ['B', 'A', 'C']}

mgaligniana commented 7 months ago

I would not talk about internal method. Instead, please briefly describe what changed for the end user

Makes sense!

Ready! Thanks for your help and patience.

beliaev-maksim commented 7 months ago

looks good, thanks

markstory commented 7 months ago

There are a few failing tests that will need to be addressed before this could be merged.

mgaligniana commented 7 months ago

There are a few failing tests that will need to be addressed before this could be merged.

Will star working on that! I'm going to use tox to can reproduce them! 👷 🚧

mgaligniana commented 7 months ago

Now it is! I missed a f-string and probably forgot run tests before push 🤷, sorry!

mgaligniana commented 6 months ago

@markstory sorry for the direct ping! But could you re-run the tests? Thank you!

markstory commented 6 months ago

Thank you 🎉