Add linter: Make sure we explicitly open all text files using UTF-8 encoding in Python by practicalswift · Pull Request #13448 · bitcoin/bitcoin

practicalswift · 2018-06-12T15:51:06Z

Add linter: Make sure we explicitly open all text files using UTF-8 encoding in Python.

As requested by @laanwj in #13440.

promag · 2018-06-12T15:56:04Z

Concept ACK.

maflcko · 2018-06-12T15:56:08Z

contrib/devtools/clang-format-diff.py

Can you upstream this? (Asking only half-serious)

Unfortunately LLVM does not use GitHub (beyond a repo mirror) and I try to limit my open source contributions to projects accepting GitHub PR:s due to time constraints :-)

maflcko · 2018-06-12T15:57:21Z

contrib/linearize/linearize-hashes.py

The cookie file is ASCII encoded

I'm not sure that's a problem: if something is ASCII-encoded, it doesn't hurt reading it as utf-8.

No strong opinion. Would throw and error if it was no longer ascii.

maflcko · 2018-06-12T15:58:49Z

test/functional/feature_notifications.py

txids should only be ascii encoded

maflcko · 2018-06-12T15:59:21Z

test/functional/test_framework/util.py

The cookie file is ASCII encoded

maflcko · 2018-06-12T16:00:39Z

test/lint/lint-python-utf8-encoding.sh

Should say encoding='utf8'? Also, you might want to use the single quotes(') when patching the python files in this pull, since this seems the more common way for short strings. Just a nit, though.

maflcko · 2018-06-12T16:01:14Z

utACK 743add2f9ee381b80f70869585cd48e89efc5df1. Just a few nits. Feel free to ignore them.

laanwj · 2018-06-12T18:02:05Z

Thanks for adding this.
~~utACK 743add2f9ee381b80f70869585cd48e89efc5df1~~, no strong opinion on ascii versus utf-8. I'm ok with being strict there, or keeping it like this, the point is being explicit.

Edit, this trips up the linter in travis:

Python's open(...) seems to be used to open text files without explicitly
specifying 'encoding=utf8':
contrib/verify-commits/verify-commits.py:    verified_root = open(dirname + "/trusted-git-root", "r").read().splitlines()[0]
contrib/verify-commits/verify-commits.py:    verified_sha512_root = open(dirname + "/trusted-sha512-root-commit", "r").read().splitlines()[0]
contrib/verify-commits/verify-commits.py:    revsig_allowed = open(dirname + "/allow-revsig-commits", "r").read().splitlines()
contrib/verify-commits/verify-commits.py:    unclean_merge_allowed = open(dirname + "/allow-unclean-merge-commits", "r").read().splitlines()
contrib/verify-commits/verify-commits.py:    incorrect_sha512_allowed = open(dirname + "/allow-incorrect-sha512-commits", "r").read().splitlines()
^---- failure generated from test/lint/lint-python-utf8-encoding.sh

…r ASCII encoding in Python

practicalswift · 2018-06-12T19:50:15Z

@MarcoFalke @laanwj Thanks for the quick review. Feedback addressed. Please re-review :-)

maflcko · 2018-06-12T20:42:12Z

utACK c8176b3

DrahtBot · 2018-06-14T20:15:58Z

Note to reviewers: This pull request conflicts with the following ones:

wallet: Replace %w by wallet name in -walletnotify script #13339 (wallet: Replace %w by wallet name in -walletnotify script by promag)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

ken2812221

utACK c8176b3

ken2812221 · 2018-06-15T13:35:30Z

test/lint/lint-python-utf8-encoding.sh

+# avoid potential issues on the BSDs where the locale is not always set.
+
+EXIT_CODE=0
+OUTPUT=$(git grep " open(" -- "*.py" | grep -vE "encoding=.(ascii|utf8|utf-8)." | grep -vE "open\([^,]*, ['\"][^'\"]*b[^'\"]*['\"]")


nit: I would prefer not to use . to match "

It matches also '. Imo every other character would be a syntax error in python, so should be fine.

@ken2812221 That is intentional to match ' and ".

Right, that makes sense.

@laanwj

… using UTF-8 encoding in Python c8176b3 Add linter: Make sure we explicitly open all text files using UTF-8 or ASCII encoding in Python (practicalswift) 634bd97 Explicitly specify encoding when opening text files in Python code (practicalswift) Pull request description: Add linter: Make sure we explicitly open all text files using UTF-8 encoding in Python. As requested by @laanwj in #13440. Tree-SHA512: 1651c00fe220ceb273324abd6703aee504029b96c7ef0e3029145901762c733c9b9d24927da281394fd4681a5bff774336c04eed01fafea997bb32192c334c06

@laanwj

…t files using UTF-8 encoding in Python c8176b3 Add linter: Make sure we explicitly open all text files using UTF-8 or ASCII encoding in Python (practicalswift) 634bd97 Explicitly specify encoding when opening text files in Python code (practicalswift) Pull request description: Add linter: Make sure we explicitly open all text files using UTF-8 encoding in Python. As requested by @laanwj in bitcoin#13440. Tree-SHA512: 1651c00fe220ceb273324abd6703aee504029b96c7ef0e3029145901762c733c9b9d24927da281394fd4681a5bff774336c04eed01fafea997bb32192c334c06 Signed-off-by: pasta <[email protected]> # Conflicts: # contrib/devtools/circular-dependencies.py # contrib/linearize/linearize-data.py # contrib/linearize/linearize-hashes.py # contrib/seeds/generate-seeds.py # contrib/verify-commits/verify-commits.py # test/functional/multiwallet.py # test/functional/notifications.py # test/functional/test_runner.py # test/util/rpcauth-test.py

@laanwj

…t files using UTF-8 encoding in Python c8176b3 Add linter: Make sure we explicitly open all text files using UTF-8 or ASCII encoding in Python (practicalswift) 634bd97 Explicitly specify encoding when opening text files in Python code (practicalswift) Pull request description: Add linter: Make sure we explicitly open all text files using UTF-8 encoding in Python. As requested by @laanwj in bitcoin#13440. Tree-SHA512: 1651c00fe220ceb273324abd6703aee504029b96c7ef0e3029145901762c733c9b9d24927da281394fd4681a5bff774336c04eed01fafea997bb32192c334c06

@laanwj

…t files using UTF-8 encoding in Python c8176b3 Add linter: Make sure we explicitly open all text files using UTF-8 or ASCII encoding in Python (practicalswift) 634bd97 Explicitly specify encoding when opening text files in Python code (practicalswift) Pull request description: Add linter: Make sure we explicitly open all text files using UTF-8 encoding in Python. As requested by @laanwj in bitcoin#13440. Tree-SHA512: 1651c00fe220ceb273324abd6703aee504029b96c7ef0e3029145901762c733c9b9d24927da281394fd4681a5bff774336c04eed01fafea997bb32192c334c06 Signed-off-by: pasta <[email protected]>

@laanwj

…t files using UTF-8 encoding in Python c8176b3 Add linter: Make sure we explicitly open all text files using UTF-8 or ASCII encoding in Python (practicalswift) 634bd97 Explicitly specify encoding when opening text files in Python code (practicalswift) Pull request description: Add linter: Make sure we explicitly open all text files using UTF-8 encoding in Python. As requested by @laanwj in bitcoin#13440. Tree-SHA512: 1651c00fe220ceb273324abd6703aee504029b96c7ef0e3029145901762c733c9b9d24927da281394fd4681a5bff774336c04eed01fafea997bb32192c334c06

fanquake added the Scripts and tools label Jun 12, 2018

maflcko reviewed Jun 12, 2018

View reviewed changes

test/functional/feature_notifications.py Outdated

Copy link

Member

maflcko Jun 12, 2018

Choose a reason for hiding this comment

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

txids should only be ascii encoded

maflcko reviewed Jun 12, 2018

View reviewed changes

test/functional/test_framework/util.py Outdated

Copy link

Member

maflcko Jun 12, 2018

Choose a reason for hiding this comment

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

The cookie file is ASCII encoded

maflcko reviewed Jun 12, 2018

View reviewed changes

Explicitly specify encoding when opening text files in Python code

634bd97

practicalswift force-pushed the lint-python-utf8-encoding branch 4 times, most recently from 2706dd2 to 3a12a51 Compare June 12, 2018 19:48

Add linter: Make sure we explicitly open all text files using UTF-8 o…

c8176b3

…r ASCII encoding in Python

practicalswift force-pushed the lint-python-utf8-encoding branch from 3a12a51 to c8176b3 Compare June 12, 2018 19:49

ken2812221 approved these changes Jun 15, 2018

View reviewed changes

laanwj merged commit c8176b3 into bitcoin:master Jun 16, 2018

Bushstar mentioned this pull request Jun 18, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#347

Merged

str4d mentioned this pull request Oct 27, 2020

Backport useful lints from upstream zcash/zcash#4829

Merged

practicalswift deleted the lint-python-utf8-encoding branch April 10, 2021 19:34

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Conversation

practicalswift commented Jun 12, 2018

Uh oh!

promag commented Jun 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Jun 12, 2018

Uh oh!

maflcko commented Jun 12, 2018

Uh oh!

DrahtBot commented Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ken2812221 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

maflcko commented Jun 12, 2018 •

edited

Loading

laanwj commented Jun 12, 2018 •

edited

Loading

DrahtBot commented Jun 14, 2018 •

edited

Loading