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.

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

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

Merged
merged 1 commit into from
May 8, 2024

Conversation

mgaligniana
Copy link
Contributor

@mgaligniana mgaligniana commented Apr 26, 2024

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
Copy link
Collaborator

@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
Copy link
Collaborator

@mgaligniana please add changelog file entry

@mgaligniana
Copy link
Contributor Author

@beliaev-maksim thank you for your fast response! On it! 🧑‍🏭

@mgaligniana
Copy link
Contributor Author

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

@markstory
Copy link
Member

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
Copy link
Contributor Author

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

@beliaev-maksim
Copy link
Collaborator

@mgaligniana yes please

@markstory
Copy link
Member

markstory commented Apr 29, 2024

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

Using this pull request would be simpler for us.

@mgaligniana mgaligniana force-pushed the issue-704 branch 2 times, most recently from 7beb706 to a6ef69d Compare April 30, 2024 04:30
@mgaligniana
Copy link
Contributor Author

mgaligniana commented Apr 30, 2024

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']}

CHANGES Outdated
Comment on lines 4 to 5
* Fixed error messages when matches fail. See #704
* Delete `_create_key_val_str` which was introduced for the compatibility between python2 (not supported anymore) and python3.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mgaligniana
Copy link
Contributor Author

mgaligniana commented Apr 30, 2024

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
Copy link
Collaborator

looks good, thanks

@markstory
Copy link
Member

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

@mgaligniana
Copy link
Contributor Author

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! 👷 🚧

Delete `_create_key_val_str` which was introduced for the compatibility
between python2 (not supported anymore) and python3.

Fixes getsentryGH-704
@mgaligniana
Copy link
Contributor Author

mgaligniana commented May 1, 2024

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

@mgaligniana
Copy link
Contributor Author

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

@markstory markstory merged commit 6791922 into getsentry:master May 8, 2024
14 checks passed
@markstory
Copy link
Member

Thank you 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error Message for failed matches sorts arrays even if not originally sorted
3 participants