Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#7830 enhancement closed fixed (fixed)

method to convert between different modes of FilePath

Reported by: Glyph Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/filepath-modeconvert-7830
branch-diff, diff-cov, branch-cov, buildbot
Author: hawkowl

Description

#7805 implemented "modal" unicode support for FilePath, but it neglected to implement a mechanism to convert between text mode and bytes mode.

Any code which accesses the path or sep attribute, or calls a method like basename which doesn't take an argument, needs to expect a FilePath in a particular mode, so it gets those values in the correct type. In order to expect a FilePath like this, there should be a way to convert and do nothing else.

We really want these methods to be on FilePath itself, so that we can implement smarter strategies in the future (for example, an undecodable path might still have a perfectly decodable basename, and if we preserve bytes internally and expose text externally we can save more information if it's a method on FilePath rather than some external construction mechanism).

These methods should return a new FilePath if a different mode is requested, since changing the mode on a shared FilePath could quite easily break things; however, since the only mutable state within a FilePath itself is cached stat information, it should be fine to return self in the case where the same mode is requested.

Change History (7)

comment:1 Changed 6 years ago by hawkowl

Author: hawkowl
Branch: branches/filepath-modeconvert-7830

(In [44448]) Branching to filepath-modeconvert-7830.

comment:2 Changed 6 years ago by hawkowl

Keywords: review added
Owner: hawkowl deleted

Changes made, builders spun, please review.

comment:3 Changed 6 years ago by Adi Roiban

Keywords: review removed
Owner: set to hawkowl

Changes looks good.

I guess that this will need another reviewer to check that the new method names make sense.

I am ok with them as I could not find a better name for them.

I don't see any tests exercising the option encoding argument ... if it is not needed maybe we can merge this without the encoding argument and add it later once an use case emerge otherwise, I think that we need tests to check that encoding argument is used.

Thanks!

comment:4 Changed 6 years ago by hawkowl

Keywords: review added
Owner: hawkowl deleted

Added some tests for the encoding argument.

Builders spun, please review.

comment:5 Changed 6 years ago by Adi Roiban

Changes looks good. Thanks!

I will leave it in the review queue so that the API is approved by another dev.

In case there is another work item depending on this, feel free to merge the current branch and continue the work.

Thanks!

comment:6 Changed 6 years ago by hawkowl

Resolution: fixed
Status: newclosed

(In [44576]) Merge filepath-modeconvert-7830: Methods to convert FilePaths to text/bytes modes

Author: hawkowl Reviewer: adiroiban Fixes: #7830

comment:7 Changed 6 years ago by hawkowl

Keywords: review removed

Since it lines up with the existing API + terminology glyph proposed, I think it should be okay.

Note: See TracTickets for help on using tickets.