-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Ensure that node ids are printable #4418
Ensure that node ids are printable #4418
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4418 +/- ##
==========================================
+ Coverage 95.88% 95.89% +<.01%
==========================================
Files 111 111
Lines 25008 25039 +31
Branches 2438 2441 +3
==========================================
+ Hits 23979 24010 +31
- Misses 725 727 +2
+ Partials 304 302 -2
Continue to review full report at Codecov.
|
else: | ||
escaped.decode("ascii") | ||
assert isinstance(escaped, six.text_type) | ||
escaped.encode("ascii") |
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.
even with the old code its not clear what good this test does
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 think it's a cheeky way to say "this is encodabe / decodable with ascii"
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.
this looks great, we need to follow up with taking off some hacks (like the null byte converted to "(null)"
in the env var
@RonnyPfannschmidt @blueyed -- is this ok to go into |
@asottile |
features please |
This was documented before, but never enforced. Passing non-strings could have strange side-effects and enforcing a string simplifies other implementation.
done, rebased against features |
Resolves #4397
targeting
features
:param(id=...)
is a stringid=...
on construction when manually passed to enforce printability