Opened 6 years ago

Closed 6 years ago

#3588 enhancement closed fixed (fixed)

Convenience method for repeated FilePath.parent()

Reported by: jml Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/parents-3588
(diff, github, buildbot, log)
Author: glyph Launchpad Bug:

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 6 years ago.
A patch which implements this, since it's pretty simple.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 6 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 6 years ago by jml

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 follow-up: Changed 6 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 ; follow-up: Changed 6 years ago by jml

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 6 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 6 years ago by glyph

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

comment:6 Changed 6 years ago by glyph

  • Keywords review added
  • Owner glyph deleted

Merry Christmas.

comment:7 follow-up: Changed 6 years ago by jml

  • 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 6 years ago by glyph

  • Author set to glyph
  • Branch set to branches/parents-3588

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

comment:9 in reply to: ↑ 7 Changed 6 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 6 years ago by glyph

  • Status changed from new to assigned

comment:11 Changed 6 years ago by glyph

  • Keywords review added
  • Owner glyph deleted
  • Status changed from assigned to new

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 6 years ago by jml

  • 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 6 years ago by jml

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 6 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 6 years ago by glyph

  • Resolution set to fixed
  • Status changed from new to closed

(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 4 years ago by <automation>

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