-
Notifications
You must be signed in to change notification settings - Fork 563
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
chore(src/compiler/msvc): replace rust-encoding with encoding-rs #2010
Conversation
Please rebase with main branch to remove unrelated commits. |
9ace132
to
33e07e2
Compare
Done. Rebased on top of the |
No, there are some differences I failed to account for when switching the implementations. |
Fixed. I also added one more test case to test the decoding logic more thoroughly. |
... since former is unmaintained
... for `test_responsefile_encoding_utf16le` since encoding_rs does not have a real UTF-16LE encoder
... for Windows 1252 code page. This tests an undefined symbol in ISO-8859-1 (but is defined in Windows-1252 and ISO-8859-15)
9844fe4
to
65512a1
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2010 +/- ##
==========================================
+ Coverage 30.49% 30.95% +0.46%
==========================================
Files 51 51
Lines 19638 19617 -21
Branches 9437 9427 -10
==========================================
+ Hits 5988 6072 +84
+ Misses 8015 7874 -141
- Partials 5635 5671 +36 ☔ View full report in Codecov by Sentry. |
CI tests passed. The coverage misses are due to negative code-path not being reached (should I add tests to test invalid encodings?) |
In general, negative test cases are also, if not more, beneficial, as arguably the error-handling codepath is more valuable regarding UX and test coverage... and in this case I think we want to be robust against inputs that definitely can contain invalid encoding, and can be controlled by the user. |
do you know if it fixes any issue ? |
In theory, any valid Windows 1252 but invalid ISO 8859-1 code points (e.g. file names) should work now. |
This pull request replaces
rust-encoding
withencoding-rs
.The former is unmaintained and uses ambiguous encoding for
ISO 8859-1
(note thatCP1252
aka "Latin-1" on Windows(R) is a superset ofISO 8859-1
). This pull request also corrects this issue by using theWINDOWS-1252
encoding to match the encoding more closely.