Open ghost opened 6 years ago
@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.
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.
Got this from a WWDC vid 2015 showing the power of URL's.
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
.
@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:
URL
's APIsURL
sOverall, though, this seems like a good direction to go.
The URL branch is starting to look better. Let's have a conversation about the tradeoffs before moving forward with a PR.
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? 🙂
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()
Yeah 👍
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.
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.
@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:
File(url: URL)
& File(path: String)
)Which direction do you like more?
@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()
.
@rob-nash I've made the branch backwards compatible with iOS 8 and tvOS 9 👍
@clayellis that would explain it! I'm always wearing my iOS hat 🤠😄
Is it worth removing some of the boiler plate equatable implementations, given that Swift now handles that?
https://github.com/JohnSundell/Files/blob/06f95bd1bf4f8d5f50bc6bb30b808c253acd4c88/Sources/Files.swift#L56 https://github.com/JohnSundell/Files/blob/06f95bd1bf4f8d5f50bc6bb30b808c253acd4c88/Sources/Files.swift#L98 etc
@rob-nash Yeah, probably 👍
Discussion
Would it be better to return instances of URL wherever possible, instead of stringPath ?