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

Added CreationDate and Size properties to Item #65

Closed
wants to merge 5 commits into from

Conversation

Gruppio
Copy link

@Gruppio Gruppio commented May 28, 2018

Hi!
I've added these 2 new properties to Item.
These new properties are optional because on some Linux systems may not be available

Copy link
Contributor

@clayellis clayellis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Gruppio these are great additions. I left one comment on size.

public private(set) lazy var creationDate: Date? = self.loadCreationDate()

/// The size of the item in bytes, or `nil` if the information is not available
public private(set) lazy var size: UInt64? = self.loadSize()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be preferred to expose size as an Int (as opposed to UInt64).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, of note, @JohnSundell's most recent article suggests that size could be more "self-documenting" by creating a typealias for Int called ByteCount to explicitly state that size is in terms of bytes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @clayellis Great advice!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay 🎉

@JohnSundell
Copy link
Owner

Thanks for this @Gruppio! 😄 I'm wondering if it's worth force-unwrapping those optionals (like we do for creationDate) to make the API more simple. You mention that it may not be available on some Linux systems, will it always return nil on Linux, or just for some versions of it?

let data = "1234567890".data(using: .utf8)!
let fileWithContent = try folder.createFile(named: "FileWithContent")
try fileWithContent.write(data: data)
if let fileWithContentSize = fileWithContent.size {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general it's not a good idea to use conditionals in unit tests, as it can often end up causing false positives (if the property isn't supposed to be nil, but actually is). You can use XCTAssertEqual with an optional, so better pass fileWithContent.size directly instead of using an if let 🙂

@Gruppio
Copy link
Author

Gruppio commented May 30, 2018

Hi @JohnSundell ! My pleasure!
I've just tried on the Uubuntu Travis CI and on that case it has returned nil. ( commit 25ee111 )
I think that the force unwrap solution is a good choice! 😄

@Gruppio
Copy link
Author

Gruppio commented May 30, 2018

@JohnSundell I've made the changes but I had to disable those 2 test for linux 😞

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

Successfully merging this pull request may close these issues.

3 participants