Use safe_str() to format warning message about unicode in Python 2 #4195
Conversation
Those were removed by accident in a previous commits it seems
nicoddemus
force-pushed
the
issue-3691
branch
from
db8083b
to
864d7fe
Compare
Codecov Report
@@ Coverage Diff @@
## master #4195 +/- ##
==========================================
+ Coverage 95.72% 95.81% +0.09%
==========================================
Files 109 109
Lines 24244 24327 +83
Branches 2390 2406 +16
==========================================
+ Hits 23207 23310 +103
+ Misses 736 721 -15
+ Partials 301 296 -5
Continue to review full report at Codecov.
|
|
What is happening with Travis that it is creating two builds for each PR, cancelling one of them immediately?
|
idk, I've been seeing this only for pytest for the last couple of weeks. Sometimes it cancels the wrong build too and then you have to manually restart it 🤦♂️ |
|
nice! what was the trick? (also fyi kinda neat feature of git: |
| @@ -18,7 +18,7 @@ envlist = | |||
|
|
|||
| [testenv] | |||
| commands = | |||
| {env:_PYTEST_TOX_COVERAGE_RUN:} pytest --lsof | |||
| {env:_PYTEST_TOX_COVERAGE_RUN:} pytest --lsof {posargs} | |||
There was a problem hiding this comment.
Why do we need {posargs} here? Left over from local testing?
There was a problem hiding this comment.
@Zac-HD without posargs its impossible to run specific tests in calls like tox -- testing/test_some.py since it wouldnt pass on
| @@ -0,0 +1,2 @@ | |||
| Python 2: safely format warning message about passing unicode strings to ``warnings.warn``, which may cause | |||
| surprising ``MemoryError`` exception when monkey patching ``warnings.warn`` itself. | |||
There was a problem hiding this comment.
(comment for my own education about pytest's documentation)
In Hypothesis we have the intersphinx_mapping configured so that we can write these as:
surprising :class:`python:MemoryError` exception when monkey
patching :func:`python:warnings.warn` itself.
It allows for somewhat nicer formatting, but more importantly adds a hyperlink - and can check that the link target exists, which caught dozens of typos in our docs.
Has pytest decided not to do this, or just not considered it? It can be fiddly at times but also pretty useful.
There was a problem hiding this comment.
i don't recall use taking a deeper look into this so it may be worth trying it - ideally someone with experience setting it up
There was a problem hiding this comment.
@Zac-HD thanks a lot for the pointer, actually or intersphinx_mapping is already configured. I tested locally your changes and they actually do work. 😁
We should remember to use those sphinx references more often. 👍
There was a problem hiding this comment.
Hmm actually I spoke too soon. While it generates the proper docs, our linter complains:
ERROR changelog/3691.bugfix.rst:1 Unknown interpreted text role "func".
ERROR changelog/3691.bugfix.rst:1 Unknown interpreted text role "class".
ERROR changelog/3691.bugfix.rst:1 Unknown interpreted text role "func".
ERROR changelog/4192.bugfix.rst:1 Unknown interpreted text role "func".
ERROR changelog/4192.bugfix.rst:1 Unknown interpreted text role "func".
| @@ -123,7 +123,7 @@ def warning_record_to_str(warning_message): | |||
| if unicode_warning: | |||
| warnings.warn( | |||
| "Warning is using unicode non convertible to ascii, " | |||
| "converting to a safe representation:\n %s" % msg, | |||
| "converting to a safe representation:\n {!r}".format(compat.safe_str(msg)), | |||
There was a problem hiding this comment.
oh, when I suggested {!r}, then you don't need safe_str at all I think
There was a problem hiding this comment.
Oh. Hmm well not sure if this will be a problem as is.
There was a problem hiding this comment.
No problem, just I was hoping to eventually remove safe_str usages 🤷♂️
Fix #3691