Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13070 )
Change subject: Make infra/python compatible with both Python 2 & 3 ...................................................................... Patch Set 2: (4 comments) These changes seem fine, thanks for fixing. There's going to be the risk of regressions until we test this pre-commit, and I don't want to ask contributors to do extra work before we've automated it, but no harm in fixing them best-effort. I think we could avoid print-related regressions with "from __future__ import print_function", so I suggested a few places to add that. Once the few minor things I suggested are fixed, looks good to merge. http://gerrit.cloudera.org:8080/#/c/13070/2/infra/python/bootstrap_virtualenv.py File infra/python/bootstrap_virtualenv.py: PS2: Maybe add "from __future__ import print_function" to prevent this regressing in future: https://stackoverflow.com/questions/32032697/how-to-use-from-future-import-print-function http://gerrit.cloudera.org:8080/#/c/13070/2/infra/python/deps/find_py26.py File infra/python/deps/find_py26.py: PS2: Maybe add "from __future__ import print_function" to prevent this regressing in future: https://stackoverflow.com/questions/32032697/how-to-use-from-future-import-print-function http://gerrit.cloudera.org:8080/#/c/13070/2/infra/python/deps/pip_download.py File infra/python/deps/pip_download.py: PS2: Maybe add "from __future__ import print_function" to prevent this regressing in future: https://stackoverflow.com/questions/32032697/how-to-use-from-future-import-print-function http://gerrit.cloudera.org:8080/#/c/13070/2/infra/python/deps/pip_download.py@32 PS2, Line 32: try: Can you add a one line comment to explain this? -- To view, visit http://gerrit.cloudera.org:8080/13070 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If4285a021bb581f88425daa52ef8a3f844017d82 Gerrit-Change-Number: 13070 Gerrit-PatchSet: 2 Gerrit-Owner: Akshesh Doshi <aksheshdo...@gmail.com> Gerrit-Reviewer: Akshesh Doshi <aksheshdo...@gmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Mon, 22 Apr 2019 20:49:07 +0000 Gerrit-HasComments: Yes
