Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ContainsFile API #60

Closed
ghost opened this issue May 21, 2018 · 5 comments
Closed

ContainsFile API #60

ghost opened this issue May 21, 2018 · 5 comments

Comments

@ghost
Copy link

ghost commented May 21, 2018

When asking if a folder contains a file, the API is currently

let folder = try Folder(named: "Foo")
let contains: Bool = folder.containsFile(named: "NameOfFile")

Could we extend this to handle an actual file variable instead?

let file: File = ...
let folder = try Folder(named: "Foo")
let contains: Bool = folder.containsFile(file)

An interesting point to add: I was getting a false result when using name instead of nameExcludingExtension.

let name = file.nameExcludingExtension
let doesContain = folder.containsFile(named: name) // true
let name = file.name
let doesContain = folder.containsFile(named: name) // false

I can understand why this happens, but it was unexpected at first.

@clayellis
Copy link
Contributor

clayellis commented May 21, 2018

In order to stay in line with the Foundation APIs, let's change the method signature from containsFile(_ file: File) to contains(_ file: File).

Great addition. Seems like something that should've always been there.

@clayellis
Copy link
Contributor

@rob-nash Are you working on a PR or is this open to anyone for implementation?

@JohnSundell
Copy link
Owner

Sounds like a great addition 👍 Agree with @clayellis that the new method should be called contains(_ file: File), and we could (not necessarily now) also add a contains(_ folder: Folder) overload as well.

@ghost
Copy link
Author

ghost commented May 22, 2018

@clayellis No, this is open for implementation. 👍🏼

@clayellis
Copy link
Contributor

clayellis commented May 24, 2018

Starting an PR for this (Update: #64)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants