URL vs String

[ghost]

Discussion

Would it be better to return instances of URL wherever possible, instead of stringPath ?

URL objects are the preferred way to refer to local files. Most objects that read data from or write data to a file have methods that accept an NSURL object instead of a pathname as the file reference. For example, you can get the contents of a local file URL as an NSString object using the init(contentsOf:encoding:) initializer, or as an NSData object using the init(contentsOf:options:) initializer.

You can also use URLs for interapplication communication. In macOS, the NSWorkspace class provides the open(:) method to open a location specified by a URL. Similarly, in iOS, the UIApplication class provides the openURL(:) method.

Additionally, you can use URLs when working with pasteboards, as described in NSURL Additions Reference (part of the AppKit framework).

The NSURL class is “toll-free bridged” with its Core Foundation counterpart, CFURL. See Toll-Free Bridging for more information on toll-free bridging.

[clayellis]

@JohnSundell what do you think about this change? It makes sense to me. We've come across bugs in the past (due to missing /'s) that could have easily been prevented using URL's .appendingPathComponent(_:) and could be prevented in the future.

This could be a relatively minimal change with a few conveniences (such as this tip) and potentially a few private operators to combine urls (not a fan of new operators, but could help with preventing introducing too many changes to the broader codebase.)

If we pull it off just right, we might be able to do this without any breaking changes to the public-facing API.

[clayellis]

After a deeper dive, looks like FileManager itself relies heavily on String paths which may be enough precedent to stick with String. I'd still be interested in hearing your thoughts, @JohnSundell.

[ghost]

Got this from a WWDC vid 2015 showing the power of URL's.

[clayellis]

Right, URL has a very robust API. I love using it. I wonder why FileManager uses String everywhere, though. Most of its methods have a URL counterpart, but not all.

I'm going to take an hour right now and see how much can be done to replace String with URL.

[clayellis]

@rob-nash @JohnSundell I've started a WIP branch for the conversion.

This is an extremely preliminary branch with all tests passing. A few of the public APIs were broken (with plans to undo those breaking changes) but were fixed with a couple internal extensions (that we might want to consider making public).

My initial feelings after doing this are:

• 👍 This is way safer than it was previously thanks to URL's APIs
• 👍 I was able to cut out a ton of failure-prone work like expanding paths, inspecting path components, manipulation of substrings, etc.
• 👍 Everything felt really natural and less mysterious when using URLs
• 👎 Without these extensions the framework is hard to work with. That could be resolved by just making them public. (Edit: I've made them public now)

Overall, though, this seems like a good direction to go.

[clayellis]

The URL branch is starting to look better. Let's have a conversation about the tradeoffs before moving forward with a PR.

[JohnSundell]

The reason I chose to base the API on strings is mostly for convenience (since Files was primarily made for scripting, which is a context in which - in general - convenience is favored over strict "correctness"). That being said, I'm not opposed to changing the internals to be URL-based (and we can provide overloads that accept URLs instead of strings on the top level as well), as long as the convenience of the string-based system is still there (I wouldn't want to write all the URL(string:) boilerplate in my scripts). Sounds like a good middle ground? 🙂

[ghost]

That makes sense @JohnSundell. We could provide public API that offers both? So you can pass in a URL or a string. Apple seems to acknowledge the usefulness of both approaches.

Bundle.main.url(forResource: "", withExtension: "") 
Bundle.main.path(forResource: "", ofType: "")

FileManager.default.moveItem(atPath: "", toPath: "")
FileManager.default.moveItem(at: URL(), to: URL()

[JohnSundell]

Yeah 👍

[ghost]

Hi @clayellis

The url branch isn't stable at this very moment in time. The target is set to iOS 8 but the code is touching iOS 9 API.

I guess bumping the deployment target is the first noticeable trade-off.

I'll bump the deployment target locally, so that it compiles on my machine but I won't be making any contributions to your branch right now.

At first glance, it's looking good.

[ghost]

Just bumped to 9 but no dice. iOS 10 API also being used. I suppose we should talk about the minimum deployment target at this point.

[clayellis]

@rob-nash I haven't tested targeting iOS, I've just been targeting macOS while playing around.

@JohnSundell yeah, that's what I've felt, too. Files is primarily meant for scripting and having to deal in URLs is a pain whereas passing in Strings is super convenient. I'm just wondering which route we want to take:

1. Expose this URL extension publicly so that Strings can be passed wherever URLs are expected.
   or
2. Hide that extension, but create counterpart methods, one for URL, one for String (i.e.: File(url: URL) & File(path: String))

Which direction do you like more?

[clayellis]

@JohnSundell is there a reason why the home directory is accessed through environment variables instead of NSHomeDirectory()? I feel like that may have been on purpose (to inject a home directory while scripting maybe?), but just want to make sure.

All of the tests pass when using NSHomeDirectory().

[clayellis]

@rob-nash I've made the branch backwards compatible with iOS 8 and tvOS 9 👍

[ghost]

@clayellis that would explain it! I'm always wearing my iOS hat 🤠😄

[ghost]

Is it worth removing some of the boiler plate equatable implementations, given that Swift now handles that?

Files/Sources/Files.swift

Line 56 in 06f95bd

public static func ==(lhs: PathError, rhs: PathError) -> Bool {

Files/Sources/Files.swift

Line 98 in 06f95bd

public static func ==(lhs: OperationError, rhs: OperationError) -> Bool {

etc

[clayellis]

@rob-nash Yeah, probably 👍