Skip to content

Conversation

@tambry
Copy link
Contributor

@tambry tambry commented May 12, 2019

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

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

blueyed commented May 12, 2019

Interesting - do you have a reference?

There are more occurences of this however, e.g. in testing/test_warnings.py.

@codecov
Copy link

codecov bot commented May 12, 2019

Codecov Report

Merging #5252 into master will decrease coverage by 3.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
testing/test_warnings.py 95.62% <ø> (-3.28%) ⬇️
src/_pytest/assertion/truncate.py 92.59% <ø> (+0.13%) ⬆️
testing/test_modimport.py 100% <ø> (ø) ⬆️
testing/test_terminal.py 98.13% <ø> (ø) ⬆️
src/_pytest/fixtures.py 97.93% <ø> (ø) ⬆️
src/_pytest/_io/saferepr.py 80.43% <ø> (ø) ⬆️
testing/test_conftest.py 99.62% <ø> (ø) ⬆️
src/_pytest/pastebin.py 90.76% <ø> (+0.14%) ⬆️
testing/test_parseopt.py 97.97% <ø> (ø) ⬆️
src/_pytest/cacheprovider.py 97.84% <ø> (ø) ⬆️
... and 116 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a43c8c...b81173e. Read the comment docs.

@tambry
Copy link
Contributor Author

tambry commented May 12, 2019

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.

@asottile
Copy link
Member

unless there's a stacktrace I'm inclined to leave this as-is -- PEP 263 seems to allow what we have currently and utf8 is properly regisetered as an alias:

>>> codecs.lookup('utf8')
<codecs.CodecInfo object for encoding utf-8 at 0x7faf98f0fca0>

@nicoddemus
Copy link
Member

Thanks @tambry!

Normalized all encoding declarations to the same format, it doesn't hurt being consistent. 👍

@@ -1,4 +1,4 @@
# coding=utf8
Copy link
Member

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!)

Copy link
Member

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? 😁

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

Copy link
Contributor

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?

Copy link
Member

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

Copy link
Contributor

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?

Copy link
Member

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

Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @asottile!

@asottile
Copy link
Member

hmmm, that's weird it's showing a diff in CI -- let me see what I can do there, sorry about that!

@asottile
Copy link
Member

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

@tambry
Copy link
Contributor Author

tambry commented May 15, 2019

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?

@asottile
Copy link
Member

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?

@tambry
Copy link
Contributor Author

tambry commented May 15, 2019

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. 👍

@nicoddemus
Copy link
Member

Waiting for @blueyed's approval/change requests before merging.

@blueyed
Copy link
Contributor

blueyed commented May 17, 2019

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.. :)

@nicoddemus
Copy link
Member

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. 👍 😁

@nicoddemus nicoddemus merged commit 66f20b6 into pytest-dev:master May 23, 2019
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.

4 participants