-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Fix invalid Python file encoding "utf8" #5252
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
Conversation
Since Python 3 it must be "utf-8", which is the official name. This is backwards compatible with Python 2.
|
Interesting - do you have a reference? There are more occurences of this however, e.g. in testing/test_warnings.py. |
Codecov Report
@@ Coverage Diff @@
## master #5252 +/- ##
==========================================
- Coverage 94.99% 91.94% -3.05%
==========================================
Files 115 115
Lines 26159 26199 +40
Branches 2578 2578
==========================================
- Hits 24849 24088 -761
- Misses 1003 1791 +788
- Partials 307 320 +13
Continue to review full report at Codecov.
|
|
I unfortunately do not have a reference. It seems to be a bit flaky whether the error happens. Though you can find cases of this problem on Google. I fixed only this occurence, because in my initial search only it showed up on Chromium CodeSearch. |
|
unless there's a stacktrace I'm inclined to leave this as-is -- PEP 263 seems to allow what we have currently and >>> codecs.lookup('utf8')
<codecs.CodecInfo object for encoding utf-8 at 0x7faf98f0fca0> |
|
Thanks @tambry! Normalized all encoding declarations to the same format, it doesn't hurt being consistent. 👍 |
| @@ -1,4 +1,4 @@ | |||
| # coding=utf8 | |||
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.
let's use fix-encoding-pragma from pre-commit/pre-commit-hooks
(yep, there's a pre-commit hook for everything!)
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.
Sure, sounds good! Could you please contribute that in place of this PR then? 😁
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.
done!
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.
Can it only do it if necessary maybe?
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.
for consistency it's all or none, that way you don't have to think about it
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.
Isn't the main point of pre-commit already that "you don't have to think about it", i.e. you do not have to care, and it will insert/request it if necessary?
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.
yeah so now you don't have to futz with whether a file has it or not, it always has it
asottile
left a comment
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.
nicoddemus
left a comment
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.
Thanks @asottile!
|
hmmm, that's weird it's showing a diff in CI -- let me see what I can do there, sorry about that! |
I suspect crlf since it's windows and that's not implemented yet -- easy patch though: pre-commit/pre-commit-hooks#384 |
|
Thanks for continuing work on this! But I think this misses all the cases that are in the middle of code somewhere. Might be worth running grep and fixing those manually? |
yep, did that already -- did I miss some? |
Ah, probably fine then. 👍 |
|
Waiting for @blueyed's approval/change requests before merging. |
I don't like changing that many files unnecessarily (https://github.com/pytest-dev/pytest/pull/5252/files#r285079054). But feel free to ignore me, of course.. :) |
|
Personally I don't mind when the change is a single line too much; wide sweeping changes like formatting annoy me more as they make 'git blame' harder to use. 👍 😁 |

Since Python 3 it must be "utf-8", which is the official name.
This is backwards compatible with Python 2.