-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 warning about non-ascii warnings even when they are ascii #2810
Conversation
_pytest/warnings.py
Outdated
@@ -73,7 +73,7 @@ def catch_warnings_for_item(item): | |||
|
|||
if compat._PY2 and any(isinstance(m, compat.UNICODE_TYPES) for m in warn_msg.args): | |||
new_args = [compat.safe_str(m) for m in warn_msg.args] | |||
unicode_warning = warn_msg.args != new_args | |||
unicode_warning = list(warn_msg.args) != new_args |
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.
It was always issue the "unicode message non-ascii compatible" because warns_msg.args
is a tuple
, which always compares as different than a list
.
interesting find ^^ |
@@ -72,8 +72,8 @@ def catch_warnings_for_item(item): | |||
unicode_warning = False | |||
|
|||
if compat._PY2 and any(isinstance(m, compat.UNICODE_TYPES) for m in warn_msg.args): | |||
new_args = [compat.safe_str(m) for m in warn_msg.args] |
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.
We didn't realize at the time, but this is incorrect, safe_str
will return utf-8
encoded bytes
, which is not ascii
.
_pytest/warnings.py
Outdated
@@ -72,8 +72,8 @@ def catch_warnings_for_item(item): | |||
unicode_warning = False | |||
|
|||
if compat._PY2 and any(isinstance(m, compat.UNICODE_TYPES) for m in warn_msg.args): | |||
new_args = [compat.safe_str(m) for m in warn_msg.args] | |||
unicode_warning = warn_msg.args != new_args | |||
new_args = [m.encode('ascii', 'replace') for m in warn_msg.args] |
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.
i fear this one could count as a breaking change as output characters are removed now
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.
You are absolutely right, thanks. Rebased.
Can we merge this @RonnyPfannschmidt ? |
@nicoddemus please add the changelog entry prefix for a deprecation/removal as this one is a bigger one alltho i still don't see why we cant just escape |
What do you mean? I'm open to suggestions here. |
@nicoddemus i dont see why we dont keep escaping as safe-string, why remove the unicode escapes as far as i can tell it shouldnbt affect ascii, but it should affect non-ascii that way - just i mean why not just use unicode-escape instead of safe_string for any unicode element? |
Fair enough, I should have mentioned this: if we use
Because of this, |
That's a good suggestion! Used Also renamed |
Fix #2809