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

Support os.walk(dir_fd=) #59391

Closed
larryhastings opened this issue Jun 25, 2012 · 9 comments
Closed

Support os.walk(dir_fd=) #59391

larryhastings opened this issue Jun 25, 2012 · 9 comments
Assignees
Labels
type-feature A feature request or enhancement

Comments

@larryhastings
Copy link
Contributor

BPO 15186
Nosy @birkenfeld, @rhettinger, @larryhastings, @serhiy-storchaka
Files
  • larry.os.walk.dir_fd.1.diff: Patch Support "bpo-" in Misc/NEWS #1 adding dir_fd to os.walk().
  • 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 = 'https://github.com/larryhastings'
    closed_at = <Date 2013-03-28.10:12:04.805>
    created_at = <Date 2012-06-25.23:39:44.634>
    labels = ['type-feature']
    title = 'Support os.walk(dir_fd=)'
    updated_at = <Date 2013-03-28.10:12:04.803>
    user = 'https://github.com/larryhastings'

    bugs.python.org fields:

    activity = <Date 2013-03-28.10:12:04.803>
    actor = 'georg.brandl'
    assignee = 'larry'
    closed = True
    closed_date = <Date 2013-03-28.10:12:04.805>
    closer = 'georg.brandl'
    components = []
    creation = <Date 2012-06-25.23:39:44.634>
    creator = 'larry'
    dependencies = []
    files = ['26160']
    hgrepos = []
    issue_num = 15186
    keywords = ['patch']
    message_count = 9.0
    messages = ['164023', '164031', '164037', '164040', '164044', '164045', '164135', '172217', '185417']
    nosy_count = 5.0
    nosy_names = ['georg.brandl', 'rhettinger', 'larry', 'Arfrever', 'serhiy.storchaka']
    pr_nums = []
    priority = 'normal'
    resolution = 'rejected'
    stage = 'patch review'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue15186'
    versions = ['Python 3.4']

    @larryhastings
    Copy link
    Contributor Author

    Adds dir_fd support to os.walk(). Just re-yields from os.fwalk(), removing the last element.

    Note: Python 3.4.

    @larryhastings larryhastings self-assigned this Jun 25, 2012
    @larryhastings larryhastings added the type-feature A feature request or enhancement label Jun 25, 2012
    @rhettinger
    Copy link
    Contributor

    Can you elaborate on why this is needed?

    The os.path.walk() API is already somewhat complex. Additional features can make the tool less usable/learnable for most users.

    @serhiy-storchaka
    Copy link
    Member

    Can you elaborate on why this is needed?

    1. Unification with fwalk and other os functions.
    2. Performance. Larry, can you use fwalk even for dir_fd is None (if fwalk exists, of cause)?

    @larryhastings
    Copy link
    Contributor Author

    Actually I think Raymond makes a good point.

    Re: symmetry: tbh that's nonsense. The reason for symmetry among functions in the os module is because they do similar things--but this is because "form follows function". We didn't decide to decorate functions with extra parameters just so they'd look nice.

    The reason you want a function to support "dir_fd" is to make it safer; using functions taking "dir_fd" and some careful programming, you can prevent some forms of timing attack. But we can't fix os.walk to make it safe in this way--which is why we have os.fwalk in the first place! So users of os.walk with this problem simply don't need "dir_fd"--they need os.fwalk.

    Re: performance: if some people care about performance here, and this approach is faster, then those people can just call os.fwalk directly. This approach to os.walk(dir_fd=) just calls os.fwalk--so calling it directly could only be even faster.

    (This is assuming my favored implementation which just calls os.fwalk--which is simple, and leverages os.fwalk doing a proper safe job of it. If we use my hackier previous version, I suspect it only made os.walk slower. Of course all of this is silly microbenchmarking anyway, and I'm not sure that these fiddling implementation details of os.walk / os.fwalk contribute to their runtime cost in any significant way.)

    I counter-propose adding some text to os.walk steering people to os.fwalk for "advanced usage". That might even be appropriate for 3.3 (beta 2). I realize they are neighbors in the documentation, but it might save at least one benighted myopic soul.

    I further propose to leave this issue open for now--there's no rush, really--and see if a compelling reason for os.walk(dir_fd=) appears. If none does, then in the fullness of time we can close this as wontfix and move on, living our lives in the dazzling sunshine of righteous truth and justice.

    Your move, Serhiy ;-)

    p.s. Serhiy: yes, you can call os.fwalk() with dir_fd=None. It would be *awful* if you could not! And actually, os.fwalk didn't even support dir_fd until... 26 hours ago.

    @larryhastings
    Copy link
    Contributor Author

    p.s. Raymond: fwiw, I think "makes it easy / hard for beginners" is only of secondary importance. Certainly I think it's reasonable to point out in a discussion, and if the beginners are easy to accommodate then okay. But if there was something that was a big win in most ways but made life harder for beginners, imo the beginners would have to take it on the chin.

    I reflect now and then on the fascinating point you made on Radio Free Python--"Unicode is now day-0 knowledge for Python"--but I have no idea what to do about it. Or about the larger point of accommodating beginners in the face of mounting language complexity. *shrug*

    My point is, I agree that os.walk(dir_fd=) probably shouldn't happen--but that's simply because we don't need it. Whether or not it made life easier or harder for beginners didn't really figure into my consideration.

    @larryhastings
    Copy link
    Contributor Author

    Whoops, you wrote "for most users" rather than talking about beginners. Sorry if I was responding to a point you weren't making ;-) I guess I assumed you were coming at this at least partially while wearing your "Python lecturer" hat.

    @serhiy-storchaka
    Copy link
    Member

    Your move, Serhiy ;-)

    It seems that you play for both sides in the last days. ;-) I surrender. Really, I'm not interested in this feature, but as of symmetry, and this is a very weak motive.

    p.s. Serhiy: yes, you can call os.fwalk() with dir_fd=None. It would be *awful* if you could not! And actually, os.fwalk didn't even support dir_fd until... 26 hours ago.

    See bpo-15200.

    @serhiy-storchaka
    Copy link
    Member

    Should this issue be closed?

    @birkenfeld
    Copy link
    Member

    Closing (it seems everybody agreed it's not a good idea).

    @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
    Labels
    type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants