bpo-38347: find Python scripts whose name contain a '-' #16536

rpluem · 2019-10-02T09:42:06Z

Allow Python scripts to contain a '-' in their filename when working
recursively.

https://bugs.python.org/issue38347

the-knights-who-say-ni · 2019-10-02T09:42:09Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@rpluem

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

vstinner

Would you mind to add a test to Lib/test/test_tools/test_pathfix.py?

rpluem · 2019-10-02T10:54:29Z

Would you mind to add a test to Lib/test/test_tools/test_pathfix.py?

This requires a bigger amount of work since the ability of pathfix.py to recursively search a directory is not tested at all currently. But the issue only happens in this case. If you specify an explicit filename on command line with '-' in the name things work fine.

vstinner · 2019-10-02T10:57:57Z

This requires a bigger amount of work since the ability of pathfix.py to recursively search a directory is not tested at all currently. But the issue only happens in this case.

It sounds like a nice enhancement to write a test for this code path.

By the way, I don't see the point of using a regex to match the filename. Why not only checking for the ".py" suffix?

rpluem · 2019-10-02T11:01:39Z

This requires a bigger amount of work since the ability of pathfix.py to recursively search a directory is not tested at all currently. But the issue only happens in this case.

It sounds like a nice enhancement to write a test for this code path.

By the way, I don't see the point of using a regex to match the filename. Why not only checking for the ".py" suffix?

I tried to keep the changes to a minimum. Of course it can be changed to allow all filenames that just have the .py suffix. I can adjust this. Whatever preference you have :-)

vstinner · 2019-10-02T11:10:22Z

The regex is quite old:

commit 9af22a037fc961b51651324bdd17a43567a688fd
Author: Guido van Rossum <guido@python.org>
Date:   Fri Aug 19 15:02:57 1994 +0000

    newslist.py: Added search for .newslistrc.py;
    pindent.py: use /usr/local/bin/python;
    pathfix.py: new script to fix #! lines in a group of scripts.

(...)

+ispythonprog = regex.compile('^[a-zA-Z0-9_]+\.py$')
+def ispython(name):
+       return ispythonprog.match(name) >= 0
+

(...)

I don't see any rationale for rejecting filenames which don't match '^[a-zA-Z0-9_]+.py$', I suggest to only check for the suffix.

vstinner · 2019-10-02T11:11:08Z

By the way, please add a NEWS entry using the Python tool "blurb" ("python3 -m pip install --user blurb" then run "blurb").

rpluem · 2019-10-02T11:16:52Z

By the way, please add a NEWS entry using the Python tool "blurb" ("python3 -m pip install --user blurb" then run "blurb").

I already did: 7aa6c09
Looks like the check status is outdated.

rpluem · 2019-10-02T12:25:02Z

This requires a bigger amount of work since the ability of pathfix.py to recursively search a directory is not tested at all currently. But the issue only happens in this case.

It sounds like a nice enhancement to write a test for this code path.

Test case created (05f15f8). I hope the code is not too weird.

rpluem · 2019-10-02T12:26:52Z

I don't see any rationale for rejecting filenames which don't match '^[a-zA-Z0-9_]+.py$', I suggest to only check for the suffix.

Done in 0c1e65c

Allow Python scripts to contain a '-' in their filename when working recursively.

Assume all files that end on '.py' are Python scripts when working recursively.

Add a test case for the recursive search including filenames with a '-'.

Fix a test failure on Windows by using os.sep instead of the Unix specific '/' as path separator.

Lib/test/test_tools/test_pathfix.py

Misc/NEWS.d/next/Tests/2019-10-02-12-27-58.bpo-38347.zdkCBX.rst

Misc/NEWS.d/next/Tools/Demos/2019-10-02-09-48-42.bpo-38347.2Tq5D1.rst

bedevere-bot · 2019-10-02T14:17:11Z

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.

No functional change.

vstinner

I see that you updated your PR, but setUp() still creates a temporary directory (I asked to move that into the test_recursive() method), there is still "**kwargs", and now test_recursive() inspects self to call other functions... I don't understand what you are trying to do.

My expectation is that test_recursive() creates a temporary directory which contains one ".py" file which has a shebang, you can pathfix on the directory, and then you check that the .py file shebang is updated.

Please don't try to magically make other test methods work on a temporary directory. I prefer simple tests.

rpluem · 2019-10-08T07:10:06Z

After reverting the previous changes to test_pathfix.py I hope that 92384a5 now addresses your concerns.

Lib/test/test_tools/test_pathfix.py

Use local variables with better names instead of class attributes.

Misc/NEWS.d/next/Tools/Demos/2019-10-02-09-48-42.bpo-38347.2Tq5D1.rst

Lib/test/test_tools/test_pathfix.py

Co-Authored-By: Victor Stinner <vstinner@python.org>

rpluem · 2019-10-11T10:12:40Z

The Azure failure seems to be caused by the inability to download OpenSSL sources for the Ubuntu tests. Maybe a temporary network issue on Azure / Openssl side. I would like to reexecute the Azure pipeline, but I don't know how.

vstinner · 2019-10-11T12:50:06Z

I closed/reopened the PR to schedule a new Azure Pipelines run.

vstinner

LGTM, but please fix the typo. It seems like I'm not allowed to push into your branch.

Lib/test/test_tools/test_pathfix.py

Co-Authored-By: Victor Stinner <vstinner@python.org>

miss-islington · 2019-10-11T13:37:40Z

Thanks @rpluem for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

…ythonGH-16536) pathfix.py: Assume all files that end on '.py' are Python scripts when working recursively. (cherry picked from commit 2b7dc40) Co-authored-by: Ruediger Pluem <r.pluem@gmx.de>

bedevere-bot · 2019-10-11T13:37:54Z

GH-16718 is a backport of this pull request to the 3.8 branch.

…H-16536) pathfix.py: Assume all files that end on '.py' are Python scripts when working recursively. (cherry picked from commit 2b7dc40) Co-authored-by: Ruediger Pluem <r.pluem@gmx.de>

…H-16536) (GH-16719) pathfix.py: Assume all files that end on '.py' are Python scripts when working recursively. (cherry picked from commit 2b7dc40)

…ythonGH-16536) pathfix.py: Assume all files that end on '.py' are Python scripts when working recursively.

the-knights-who-say-ni added the CLA not signed label Oct 2, 2019

bedevere-bot added the awaiting review label Oct 2, 2019

vstinner reviewed Oct 2, 2019

View changes

rpluem and others added 6 commits Oct 2, 2019

bpo-38347: find Python scripts whose name contain a '-'

94047a5

Allow Python scripts to contain a '-' in their filename when working recursively.

📜🤖 Added by blurb_it.

d8e988e

bpo-38347: assume all files that end on '.py' are Python scripts

7ca16a2

Assume all files that end on '.py' are Python scripts when working recursively.

bpo-38347: add a test case for the recursive search

9c512f4

Add a test case for the recursive search including filenames with a '-'.

📜🤖 Added by blurb_it.

b1fa966

bpo-38347: use os.sep instead of the Unix specific '/'

2dcdef4

Fix a test failure on Windows by using os.sep instead of the Unix specific '/' as path separator.

rpluem force-pushed the fix-issue-38347 branch from aee7801 to 2dcdef4 Compare Oct 2, 2019

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Oct 2, 2019

vstinner requested changes Oct 2, 2019

View changes

bedevere-bot removed the awaiting review label Oct 2, 2019

bedevere-bot added the awaiting changes label Oct 2, 2019

rpluem added 4 commits Oct 4, 2019

bpo-38347: rename variable target_file to filename

07f7158

No functional change.

bpo-38347: remove unneeded NEWS entry

14f5aa0

bpo-38347: adjust comment to refelct recent changes

a68ad42

bpo-38347: forgot to mention pathfix.py

05c39a5

vstinner requested changes Oct 4, 2019

View changes

rpluem requested a review from vstinner Oct 8, 2019

vstinner reviewed Oct 8, 2019

View changes

Lib/test/test_tools/test_pathfix.py Outdated Show resolved Hide resolved

Lib/test/test_tools/test_pathfix.py Outdated Show resolved Hide resolved

bpo-38347: Use local variables with better names

9e29237

Use local variables with better names instead of class attributes.

rpluem requested a review from vstinner Oct 10, 2019

vstinner reviewed Oct 10, 2019

View changes

Misc/NEWS.d/next/Tools/Demos/2019-10-02-09-48-42.bpo-38347.2Tq5D1.rst Outdated Show resolved Hide resolved

vstinner requested changes Oct 10, 2019

View changes

Lib/test/test_tools/test_pathfix.py Outdated Show resolved Hide resolved

Lib/test/test_tools/test_pathfix.py Show resolved Hide resolved

rpluem and others added 2 commits Oct 10, 2019

bpo-38347: move NEWS to the correct location

a174102

bpo-38347: Use double quotes instead of single ones

95e7a2b

Co-Authored-By: Victor Stinner <vstinner@python.org>

rpluem force-pushed the fix-issue-38347 branch from bbf6547 to 95e7a2b Compare Oct 10, 2019

rpluem requested a review from vstinner Oct 10, 2019

bpo-38347: Add comment to explain the name of the script

56eaa94

vstinner closed this Oct 11, 2019

vstinner reopened this Oct 11, 2019

vstinner approved these changes Oct 11, 2019

View changes

Lib/test/test_tools/test_pathfix.py Outdated Show resolved Hide resolved

bedevere-bot added awaiting merge and removed awaiting changes labels Oct 11, 2019

Fix typo

bbab2b4

Co-Authored-By: Victor Stinner <vstinner@python.org>

vstinner merged commit 2b7dc40 into python:master Oct 11, 2019
4 checks passed

bedevere-bot removed the awaiting merge label Oct 11, 2019

vstinner added the needs backport to 3.8 label Oct 11, 2019

bedevere-bot removed the needs backport to 3.8 label Oct 11, 2019

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

bpo-38347: find pathfix for Python scripts whose name contain a '-' (p…

045d471

…ythonGH-16536) pathfix.py: Assume all files that end on '.py' are Python scripts when working recursively.

python / cpython Public

bpo-38347: find Python scripts whose name contain a '-' #16536

bpo-38347: find Python scripts whose name contain a '-' #16536

rpluem commented Oct 2, 2019 •

edited by bedevere-bot

the-knights-who-say-ni commented Oct 2, 2019

vstinner left a comment

rpluem commented Oct 2, 2019

vstinner commented Oct 2, 2019

rpluem commented Oct 2, 2019 •

edited

vstinner commented Oct 2, 2019

vstinner commented Oct 2, 2019

rpluem commented Oct 2, 2019

rpluem commented Oct 2, 2019

rpluem commented Oct 2, 2019

bedevere-bot commented Oct 2, 2019

vstinner left a comment

rpluem commented Oct 8, 2019

rpluem commented Oct 11, 2019 •

edited

vstinner commented Oct 11, 2019

vstinner left a comment

miss-islington commented Oct 11, 2019

bedevere-bot commented Oct 11, 2019

python / cpython Public

bpo-38347: find Python scripts whose name contain a '-' #16536

bpo-38347: find Python scripts whose name contain a '-' #16536

Conversation

rpluem commented Oct 2, 2019 • edited by bedevere-bot

the-knights-who-say-ni commented Oct 2, 2019

CLA Missing

vstinner left a comment

rpluem commented Oct 2, 2019

vstinner commented Oct 2, 2019

rpluem commented Oct 2, 2019 • edited

vstinner commented Oct 2, 2019

vstinner commented Oct 2, 2019

rpluem commented Oct 2, 2019

rpluem commented Oct 2, 2019

rpluem commented Oct 2, 2019

bedevere-bot commented Oct 2, 2019

vstinner left a comment

rpluem commented Oct 8, 2019

rpluem commented Oct 11, 2019 • edited

vstinner commented Oct 11, 2019

vstinner left a comment

miss-islington commented Oct 11, 2019

bedevere-bot commented Oct 11, 2019

rpluem commented Oct 2, 2019 •

edited by bedevere-bot

rpluem commented Oct 2, 2019 •

edited

rpluem commented Oct 11, 2019 •

edited