Opened 9 years ago

Closed 9 years ago

#3588 enhancement closed fixed (fixed)

Convenience method for repeated FilePath.parent()

Reported by: Jonathan Lange Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/parents-3588
branch-diff, diff-cov, branch-cov, buildbot
Author: glyph

Description

It would be nice if FilePath let you do something like:

path = FilePath(__file__).parent(3)

where the above was equivalent to:

path = FilePath(__file__).parent().parent().parent()

Having a differently named function would be OK too.

Attachments (1)

parents.patch (2.2 KB) - added by Glyph 9 years ago.
A patch which implements this, since it's pretty simple.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 9 years ago by Glyph

How about FilePath(file).parents()[3]?

This could also address #3589 with a convenient idiom rather than a specific method: if possibleParent in possibleChild.parents()...

comment:2 Changed 9 years ago by Jonathan Lange

I could definitely use that. Your suggestion implies that FilePath(x).parents()[0] === FilePath(x), which is probably the right API but a little confusing given the name.

comment:3 Changed 9 years ago by Glyph

Hmm. I think I'd actually prefer it if FilePath(x).parents()[0] == FilePath(x).parent(). This mirrors the behavior of segmentsFrom, too, in your request for isParent (you don't want isParentOrSelf(), right?) Why do you say it's the right behavior to include the path itself?

comment:4 in reply to:  3 ; Changed 9 years ago by Jonathan Lange

Replying to glyph:

Hmm. I think I'd actually prefer it if FilePath(x).parents()[0] == FilePath(x).parent(). This mirrors the behavior of segmentsFrom, too, in your request for isParent (you don't want isParentOrSelf(), right?) Why do you say it's the right behavior to include the path itself?

Because in the cases where I want to use it, I actually want to iterate over the path and all of its ancestors -- I'm looking for something.

jml

comment:5 in reply to:  4 Changed 9 years ago by Glyph

Replying to jml:

Because in the cases where I want to use it, I actually want to iterate over the path and all of its ancestors -- I'm looking for something.

I think in those cases, the extra verbosity of "chain([it], it.parents())" is useful for clarity's sake.

It occurs to me that list(it.parents())[2] is also pretty easy too; it can easily be a generator.

To strengthen my argument on these points I shall provide a patch.

Changed 9 years ago by Glyph

Attachment: parents.patch added

A patch which implements this, since it's pretty simple.

comment:6 Changed 9 years ago by Glyph

Keywords: review added
Owner: Glyph deleted

Merry Christmas.

comment:7 Changed 9 years ago by Jonathan Lange

Keywords: review removed
Owner: set to Glyph

Hey glyph,

The patch looks good -- thanks for getting to it so quickly. I've got a couple of minor comments:

  • Isn't assertEqual preferred over assertEquals?
  • I think the test would be clearer if it used more literals and less algorithm.

Thanks, jml

comment:8 Changed 9 years ago by Glyph

Author: glyph
Branch: branches/parents-3588

(In [25740]) Branching to 'parents-3588'

comment:9 in reply to:  7 Changed 9 years ago by Glyph

Replying to jml:

Hey glyph,

The patch looks good -- thanks for getting to it so quickly. I've got a couple of minor comments:

  • Isn't assertEqual preferred over assertEquals?

If you say so. I don't remember anything like that. Is this written down anywhere?

  • I think the test would be clearer if it used more literals and less algorithm.

This is pretty vague, but I do think it's pretty crappy that all the variables are one letter. So let me fix that.

comment:10 Changed 9 years ago by Glyph

Status: newassigned

comment:11 Changed 9 years ago by Glyph

Keywords: review added
Owner: Glyph deleted
Status: assignednew

I hope I got the gist of "more literals"; I couldn't really use "less algorithm", having previously used precisely the correct amount :).

comment:12 Changed 9 years ago by Jonathan Lange

Keywords: review removed
Owner: set to Glyph

Looks good to me. Please merge.

I realize now that my comment was too cryptic -- sorry. Here's what I meant. Instead of:

def test_parents(self):
    L = []
    pathobj = self.path.child("a").child("b").child("c")
    fullpath = pathobj.path
    lastpath = fullpath
    thispath = os.path.dirname(fullpath)
    while lastpath != self.root.path:
        L.append(thispath)
        lastpath = thispath
        thispath = os.path.dirname(thispath)
    self.assertEqual([x.path for x in pathobj.parents()], L)

Try something like:

def test_parents(self):
    pathobj = self.path.child("a").child("b").child("c")
    expectedParents = [
        self.path.child('a'),
        self.path.child('a').child('b'),
        ]
    self.assertEqual(
        [x.path for x in pathobj.parents()],
        [x.path for x in expectedParents])

I think the second example makes the expected behaviour of the method clearer. That said, it's your call, and I'm happy for you to merge your patch as is.

Thanks, and sorry about the roundtripping.

jml

comment:13 Changed 9 years ago by Jonathan Lange

Also, regarding assertEqual vs assertEquals, I don't know whether one is preferred over the other -- that's why I asked the question. I have memories of being corrected on this in the past, but I can't be sure.

comment:14 Changed 9 years ago by Glyph

I agree that your second example reads more cleanly. Unfortunately the test is incorrect :). .parents() returns path objects all the way back up to the root, and those have to be determined algorithmically.

comment:15 Changed 9 years ago by Glyph

Resolution: fixed
Status: newclosed

(In [26084]) Add a parents() utility method to FilePath for iterating all parents of a path back to the filesystem.

Also add this method to ZipPath, enumerating all paths back to the root of the archive.

Author: glyph Reviewer: jml Fixes #3588

comment:16 Changed 6 years ago by <automation>

Owner: Glyph deleted
Note: See TracTickets for help on using tickets.