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

tools: fix Python 3 issues in tools/icu/icutrim.py #29213

Closed
wants to merge 3 commits into from

Conversation

@cclauss
Copy link
Contributor

commented Aug 19, 2019

Fix dict.has_key() and file encoding issues in tools/icu/icutrim.py to ensure compatibility with both Python 2 and Python 3.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@srl295
srl295 approved these changes Aug 19, 2019
Copy link
Member

left a comment

LGTM. I guess we are Python 3 only?

@cclauss

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

No. ensure compatibility with both Python 2 and Python 3.

Did I miss something related to Python 2?

@srl295

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

@cclauss OK, i see that there. I'm not familiar enough with the similarities I suppose.

@cclauss

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

https://docs.python.org/3/library/functions.html#print says:

print() cannot be used with binary mode file objects.

@srl295
srl295 approved these changes Aug 19, 2019
@cclauss cclauss referenced this pull request Aug 19, 2019
29 of 41 tasks complete

@cclauss cclauss added the python label Aug 19, 2019

config["variables"]["locales"] = {}
if options.locales:
config["variables"] = config.get("variables", {})
config["variables"]["locales"] = config["variables"].get("locales", {})
config["variables"]["locales"]["only"] = options.locales.split(',')

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Aug 20, 2019

Member

This file uses 4 space indent so can you use that here too?

(Confusing really since we use 2 space indent everywhere else. Oh well.)

This comment has been minimized.

Copy link
@cclauss

cclauss Aug 21, 2019

Author Contributor

Done.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment has been minimized.

Trott added a commit that referenced this pull request Aug 21, 2019
tools: fix Python 3 issues in tools/icu/icutrim.py
PR-URL: #29213
Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@Trott

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

Landed in d937b02

@Trott Trott closed this Aug 21, 2019

@cclauss cclauss deleted the cclauss:py3-icutrim.py branch Aug 22, 2019

ronag added a commit to nxtedition/node that referenced this pull request Aug 23, 2019
tools: fix Python 3 issues in tools/icu/icutrim.py
PR-URL: nodejs#29213
Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
BridgeAR added a commit that referenced this pull request Sep 3, 2019
tools: fix Python 3 issues in tools/icu/icutrim.py
PR-URL: #29213
Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@BridgeAR BridgeAR referenced this pull request Sep 3, 2019
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
tools: fix Python 3 issues in tools/icu/icutrim.py
PR-URL: nodejs#29213
Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
tools: fix Python 3 issues in tools/icu/icutrim.py
PR-URL: nodejs#29213
Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.