bpo-37064: add option -a to Tools/Scripts/pathfix.py #15717

PatrikKopkan · 2019-09-06T13:55:59Z

this option enables to add single literal
flag to kept flags

https://bugs.python.org/issue37064

- this option enables to add single literal flag to kept flags

vstinner · 2019-09-09T13:56:00Z

Lib/test/test_tools/test_pathfix.py

+        self.assertEqual(
+            self.pathfix(
+                '#! /usr/bin/env python -s',
+                ['-i', '/usr/bin/python3', '-a', 's', ]),


I would prefer to use a different flag in the old shebang or in the -a argument. Ex: '#! /usr/bin/env python -s' and -a S.

vstinner · 2019-09-09T13:57:09Z

Tools/scripts/pathfix.py

    flags = b''
-    if keep_flags:
-        flags = parse_shebang(line)
+    if keep_flags or add_flags:


why not always calling this function? it would be simpler

vstinner · 2019-09-09T13:57:21Z

Lib/test/test_tools/test_pathfix.py

            self.pathfix(
                '#! /usr/bin/env python',
-                ['-i', '/usr/bin/python3',]),
+                ['-i', '/usr/bin/python3', ]),


i would prefer: ['-i', '/usr/bin/python3']

Tools/scripts/pathfix.py

vstinner · 2019-09-09T15:34:14Z

Lib/test/test_tools/test_pathfix.py

+                '#! /usr/bin/env python -W something',
+                ['-i', '/usr/bin/python3', '-a', 's', '-k']),
+            '#! /usr/bin/python3 -sW something',
+        )


What if -a argument contains a space? Should we raise an error?

vstinner · 2019-09-09T15:34:42Z

Lib/test/test_tools/test_pathfix.py

+                '#! /usr/bin/env python',
+                ['-i', '/usr/bin/python3', '-a', 's']),
+            '#! /usr/bin/python3 -s',
+        )


To make tests more compact you can put ")" on the previous line in all tests.

vstinner · 2019-09-09T15:35:28Z

Lib/test/test_tools/test_pathfix.py

+            self.pathfix(
+                '#! /usr/bin/env python -s',
+                ['-i', '/usr/bin/python3', '-a', 's']),
+            '#! /usr/bin/python3 -s',


I would prefer to use different flags, like -s and -a S. Otherwise, it's unclear to see which one wins.

vstinner · 2019-09-09T15:35:47Z

Lib/test/test_tools/test_pathfix.py

+            self.pathfix(
+                '#! /usr/bin/env python -v',
+                ['-i', '/usr/bin/python3', '-a', 'v', '-k']),
+            '#! /usr/bin/python3 -vv',


Ditto, I would prefer to use different flags like -s and -a S -k.

Misc/NEWS.d/next/Tools-Demos/2019-05-27-15-26-12.bpo-37064.k_SPW2.rst

vstinner · 2019-09-11T14:49:11Z

Lib/test/test_tools/test_pathfix.py

PEP 8 prefers:

Suggested change

'#! /usr/bin/python3',)

'#! /usr/bin/python3')

Same remark for changes below.

vstinner · 2019-09-11T14:49:41Z

Tools/scripts/pathfix.py

I suggest to check for b' ' in add_flags (bytes) instead, since add_flags is used later, not a.

Tools/scripts/pathfix.py

Co-Authored-By: Victor Stinner <[email protected]> Co-Authored-By: Victor Stinner <[email protected]>

Tools/scripts/pathfix.py

vstinner · 2019-09-12T15:54:02Z

Lib/test/test_tools/test_pathfix.py

+                '#! /usr/bin/env python -W something',
+                ['-i', '/usr/bin/python3', '-a', 's', '-k']),
+            '#! /usr/bin/python3 -sW something')
+        self.assertEqual(


I suggest to move this check in a separated test_xxx() method. I would prefer to check stderr to ensure that stderr contains "-a option doesn't support whitespaces".

Something like:

def check_pathfix_error(self, shebang, pathfix_flags, expected_stderr, expected_exitcode): .. self.check_pathfix_error( '#! /usr/bin/env python -W something', ['-i', '/usr/bin/python3', '-a', 's', '-k']),), "-a option doesn't support whitespaces", 2)

bedevere-bot · 2019-09-12T15:54:09Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Co-Authored-By: Victor Stinner <[email protected]>

vstinner · 2019-09-19T13:33:59Z

Lib/test/test_tools/test_pathfix.py

        proc = subprocess.run(
            [sys.executable, self.script,
             *pathfix_flags, '-n', self.temp_file],
            capture_output=True)


please use text=1 instead of .decode() below, to use the proper encoding (locale encoding, rather than always using UTF-8).

Lib/test/test_tools/test_pathfix.py

vstinner · 2019-09-19T13:37:09Z

Lib/test/test_tools/test_pathfix.py

+                ['-i', '/usr/bin/python3', '-a', 's', '-k']),
+            '#! /usr/bin/python3 -sW something')
+
+    def test_pathfix_adding_whitespaces(self):


I suggest to rename the method to: test_pathfix_adding_errors().

vstinner · 2019-09-19T13:37:29Z

Lib/test/test_tools/test_pathfix.py

        self.addCleanup(support.unlink, support.TESTFN)

-    def pathfix(self, shebang, pathfix_flags):
+    def pathfix(self, shebang, pathfix_flags, expected_returncode=0, stderr=''):


Would you mind to rename expected_returncode to just exitcode? (shorter)

vstinner · 2019-09-19T13:38:24Z

Lib/test/test_tools/test_pathfix.py

            capture_output=True)
-        self.assertEqual(proc.returncode, 0, proc)
+
+        self.assertEqual(proc.returncode, expected_returncode, proc)


Maybe check also stdout, before stderr:

self.assertEqual(proc.stderr.decode(), "", proc)

vstinner

LGTM, but can you please fix the two remaining nitpicks before I can merge it? :-)

vstinner · 2019-09-25T11:29:59Z

Lib/test/test_tools/test_pathfix.py

+            '#! /usr/bin/python3 -Rs')
+        self.assertEqual(
+            self.pathfix(
+                '#! /usr/bin/env python -W something',


I would prefer to use working flags: " -W default" for example.

vstinner · 2019-09-25T11:30:36Z

Lib/test/test_tools/test_pathfix.py

+    def test_pathfix_adding_errors(self):
+        self.pathfix(
+            '#! /usr/bin/env python -W something',
+            ['-i', '/usr/bin/python3', '-a', ' af', '-k'],


I would prefer to use working flags: ..., '-a', 'W default', ... for example.

vstinner

LGTM, thanks @PatrikKopkan ;-)

* bpo-37064: Add option -k to Tools/scripts/pathfix.py (GH-15548) Add flag -k to pathscript.py script: preserve shebang flags. (cherry picked from commit 50254ac) * bpo-37064: Add option -a to pathfix.py tool (GH-15717) Add option -a to Tools/Scripts/pathfix.py script: add flags. (cherry picked from commit 1dc1acb)

Add option -a to Tools/Scripts/pathfix.py script: add flags.

bpo-37064: add -a to Tools/Scripts/pathfix.py

211987d

- this option enables to add single literal flag to kept flags

the-knights-who-say-ni added the CLA signed label Sep 6, 2019

bedevere-bot added the awaiting review label Sep 6, 2019

PatrikKopkan changed the title ~~bpo-37064: add -a to Tools/Scripts/pathfix.py~~ bpo-37064: add option -a to Tools/Scripts/pathfix.py Sep 6, 2019

Patrik Kopkan added 2 commits September 6, 2019 18:14

make -k not implicit

301850b

rename variable add_flag to add_flags

8af95b4

vstinner reviewed Sep 9, 2019

View reviewed changes

Tools/scripts/pathfix.py Show resolved Hide resolved

adress Victor's comments

a05ce9c

vstinner reviewed Sep 9, 2019

View reviewed changes

add error when passing space to -a

0785830

vstinner reviewed Sep 11, 2019

View reviewed changes

Patrik Kopkan and others added 2 commits September 12, 2019 16:31

add examples to comment

01dd5b9

Apply suggestions from code review

9131ddf

Co-Authored-By: Victor Stinner <[email protected]> Co-Authored-By: Victor Stinner <[email protected]>

vstinner requested changes Sep 12, 2019

View reviewed changes

bedevere-bot removed the awaiting review label Sep 12, 2019

bedevere-bot added the awaiting changes label Sep 12, 2019

PatrikKopkan and others added 2 commits September 16, 2019 14:35

Update Tools/scripts/pathfix.py

c342469

Co-Authored-By: Victor Stinner <[email protected]>

test checks stderr output

110112c

vstinner requested changes Sep 19, 2019

View reviewed changes

adress Victor's comments

902b401

vstinner reviewed Sep 25, 2019

View reviewed changes

test authentic flags

0ac33c1

vstinner approved these changes Sep 25, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Sep 25, 2019

vstinner merged commit 1dc1acb into python:master Sep 25, 2019

bedevere-bot removed the awaiting merge label Sep 25, 2019

vstinner mentioned this pull request Sep 25, 2019

[3.8] bpo-37064: Add -k and -a options to pathfix.py tool #16387

Merged

jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019

bpo-37064: Add option -a to pathfix.py tool (pythonGH-15717)

06b3508

Add option -a to Tools/Scripts/pathfix.py script: add flags.

Uh oh!

bpo-37064: add option -a to Tools/Scripts/pathfix.py #15717

bpo-37064: add option -a to Tools/Scripts/pathfix.py #15717

Uh oh!

Conversation

PatrikKopkan commented Sep 6, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Sep 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

PatrikKopkan commented Sep 6, 2019 •

edited by bedevere-bot

Loading