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: EncodingWarning #512

Merged
merged 4 commits into from Mar 13, 2023
Merged

fix: EncodingWarning #512

merged 4 commits into from Mar 13, 2023

Conversation

henryiii
Copy link
Contributor

@henryiii henryiii commented Mar 10, 2023

For reference, this is PEP 597, in Python 3.10+, and happens when PYTHONWARNDEFAULTENCODING is set.

This cherry-picks @methane's #504, but adds two new commits; one that adds testing for this (using my favorite pytest settings), and one that fixes two more cases of missing encodings discovered by adding those tests.

@codecov
Copy link

codecov bot commented Mar 10, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -2.65 ⚠️

Comparison is base (e0f18dd) 72.73% compared to head (c099007) 70.09%.

❗ Current head c099007 differs from pull request most recent head c8c99a1. Consider uploading reports for the commit c8c99a1 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #512      +/-   ##
==========================================
- Coverage   72.73%   70.09%   -2.65%     
==========================================
  Files          13       13              
  Lines        1060     1060              
==========================================
- Hits          771      743      -28     
- Misses        289      317      +28     
Impacted Files Coverage Δ
src/wheel/bdist_wheel.py 51.90% <100.00%> (ø)
src/wheel/cli/pack.py 56.89% <100.00%> (-37.94%) ⬇️
src/wheel/metadata.py 96.66% <100.00%> (ø)

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@henryiii
Copy link
Contributor Author

henryiii commented Mar 11, 2023

FYI, I'm fine if you select "update by rebase" from the dropdown on the PR for my PRs, you don't need to merge, I don't mind the rebase. If a PR stays open long enough, the merges eventually make it a mess (I spent hours trying to pull the actual changes from pypa/build#361 but couldn't untangle it). Depends on the contributor, but I don't mind resetting.

@agronholm
Copy link
Contributor

FYI, I'm fine if you select "update by rebase" from the dropdown on the PR for my PRs, you don't need to merge, I don't mind the rebase. If a PR stays open long enough, the merges eventually make it a mess (I spent hours trying to pull the actual changes from pypa/build#361 but couldn't untangle it). Depends on the contributor, but I don't mind resetting.

Noted. Personally I prefer merges to the PR branch while I'm working on one.

@agronholm
Copy link
Contributor

Looks like the merge from main I did was somehow wrong, and now tests are failing. Can you fix it?

henryiii and others added 4 commits March 12, 2023 09:34
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
@henryiii
Copy link
Contributor Author

henryiii commented Mar 12, 2023

Sure. Technically de80240 is now "one more missed encoding", because the other one was fixed when factoring-out into a function in #422. That was probably where the merge messed up.

@agronholm agronholm merged commit 472e54e into pypa:main Mar 13, 2023
@agronholm
Copy link
Contributor

Thanks!

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

Successfully merging this pull request may close these issues.

None yet

3 participants