-
Notifications
You must be signed in to change notification settings - Fork 137
Unicode encoding error #627
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
Open
Ghanchu
wants to merge
4
commits into
pytest-dev:main
Choose a base branch
from
Ghanchu:unicode-encoding-error
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+30
−1
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
3d6b955
Fixing an issue with pluggy and surrogate escape characters during tr…
Ghanchu b41c07a
added test in test_tracer.py to expose changes and increase code cove…
Ghanchu c07d438
added pluggy/testing to .coveragerc
Ghanchu 4d552bc
removed pluggy/testing/* from .coveragerc and removed redundant tests…
Ghanchu File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| This change addresses an issue in pluggy that occured when running pytest with any pluggy tracing enabled when parametrized values contained surrogate escape characters. | ||
| Before, pluggy attempted to write trace messages using UTF-8 enconding, which fails for lone surrogates. Tracing now encodes lone surrogates with errors="replace" in order | ||
| to ensure that trace logging will not crash hook execution in the future. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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'm wondering about the perf impact of this. In pylint we added error handling instead of encoding / decoding each string (so we encode/decode only in case of unicoderror) because surrogate are rather rare (at least in an occidental context).
Maybe our intuition was wrong and this is the better approach, I'm ready to be surprised by a benchmark. Would you consider 1% of surrogate a proper approximation of reality for a benchmark ?
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're right, this would impact performance, and honestly I would say that 1% percent is possibly too high of an approximation. We can benchmark this and see to what degree performance is affected.