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.

Sign up for GitHub

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

Jump to bottom

pygettext ignores directories as inputfile argument #76101

Closed
insolite mannequin opened this issue Nov 2, 2017 · 7 comments
Closed

pygettext ignores directories as inputfile argument #76101

insolite mannequin opened this issue Nov 2, 2017 · 7 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes type-bug An unexpected behavior, bug, or error

Comments

@insolite
Copy link
Mannequin

insolite mannequin commented Nov 2, 2017

BPO 31920
Nosy @serhiy-storchaka, @insolite, @miss-islington
PRs
  • bpo-31920: Fix pygettext directory walk #4225
  • bpo-31920: Fixed handling directories as arguments in the pygettext script. #6259
  • [3.7] bpo-31920: Fixed handling directories as arguments in the pygettext script. (GH-6259) #6431
  • [3.6] bpo-31920: Fixed handling directories as arguments in the pygettext script. (GH-6259) #6432
  • [2.7] bpo-31920: Fixed handling directories as arguments in the pygettext script. (GH-6259) #6436
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
closed_at = <Date 2018-04-10.08:04:26.762>
created_at = <Date 2017-11-02.08:37:20.895>
labels = ['3.8', 'type-bug', '3.7']
title = 'pygettext ignores directories as inputfile argument'
updated_at = <Date 2018-04-10.08:04:26.720>
user = 'https://github.com/insolite'

    bugs.python.org fields:

    activity = <Date 2018-04-10.08:04:26.720>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = True
closed_date = <Date 2018-04-10.08:04:26.762>
closer = 'serhiy.storchaka'
components = ['Demos and Tools']
creation = <Date 2017-11-02.08:37:20.895>
creator = 'Oleg Krasnikov'
dependencies = []
files = []
hgrepos = []
issue_num = 31920
keywords = ['patch']
message_count = 7.0
messages = ['305406', '305479', '305488', '315140', '315143', '315144', '315162']
nosy_count = 3.0
nosy_names = ['serhiy.storchaka', 'Oleg Krasnikov', 'miss-islington']
pr_nums = ['4225', '6259', '6431', '6432', '6436']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue31920'
versions = ['Python 2.7', 'Python 3.6', 'Python 3.7', 'Python 3.8']
    @insolite
    Copy link
    Mannequin Author

    insolite mannequin commented Nov 2, 2017

    This happens because pygettext's getFilesForName calls os.walk as if it was os.path.walk. But the walk function has changed signature when moved from os.path to os. So now _visit_pyfiles is passed to walk as topdown argument which is obviously wrong and therefore _visit_pyfiles is never called.

    @insolite insolite mannequin added the type-bug An unexpected behavior, bug, or error label Nov 2, 2017
    @serhiy-storchaka
    Copy link
    Member

    Thank you for your patch Oleg. 3.4 and 3.5 are in security fixes only mode now.

    Good catch, the usage of os.walk() in pygettext.py is incorrect. But your change is not enough. In _visit_pyfiles() the name 'CVS' is removed from the names list. If names is a list of directories emitted by os.walk(), this would exclude the whole directory CVS from searching. But if it is a new list "dirs + files", this doesn't have any effect. And directories with the ".py" extension shouldn't be added to the list of Python files. Hence _visit_pyfiles should take two separate lists for directories and files. And since it no longer is a callback, it would be better to inline its code.

    There are tests for pygettext in Lib/test/test_tools/test_i18n.py. It would be nice to add a new test for the fixed feature.

    @serhiy-storchaka serhiy-storchaka added the 3.7 (EOL) end of life label Nov 3, 2017
    @insolite
    Copy link
    Mannequin Author

    insolite mannequin commented Nov 3, 2017

    Thanks for quite sensible notes Serhiy. I've fixed all that in recent commit and added a regression test. Still not sure about "testing conventions" here cause this is my first PR to python repository, so please let me know if something is wrong and I'll fix that.

    @serhiy-storchaka
    Copy link
    Member

    New changeset c93938b by Serhiy Storchaka in branch 'master':
    bpo-31920: Fixed handling directories as arguments in the pygettext script. (GH-6259)
    c93938b

    @miss-islington
    Copy link
    Contributor

    New changeset 9b25bd6 by Miss Islington (bot) in branch '3.7':
    bpo-31920: Fixed handling directories as arguments in the pygettext script. (GH-6259)
    9b25bd6

    @miss-islington
    Copy link
    Contributor

    New changeset e0dbc57 by Miss Islington (bot) in branch '3.6':
    bpo-31920: Fixed handling directories as arguments in the pygettext script. (GH-6259)
    e0dbc57

    @serhiy-storchaka
    Copy link
    Member

    New changeset a61f5da by Serhiy Storchaka in branch '2.7':
    [2.7] bpo-31920: Fixed handling directories as arguments in the pygettext script. (GH-6259) (GH-6436)
    a61f5da

    @serhiy-storchaka serhiy-storchaka added the 3.8 only security fixes label Apr 10, 2018
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    3.7 (EOL) end of life 3.8 only security fixes type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    2 participants
    @serhiy-storchaka @miss-islington