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.

Sign up for GitHub

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

Jump to bottom

Use other method to enumerate contents, which causing crash(maybe only occurred for archved build) #620

Merged
merged 1 commit into from
Mar 9, 2017
Merged

Use other method to enumerate contents, which causing crash(maybe only occurred for archved build) #620

merged 1 commit into from
Mar 9, 2017

Conversation

mono0926
Copy link
Contributor

@mono0926 mono0926 commented Mar 9, 2017

@mono0926
Copy link
Contributor Author

mono0926 commented Mar 9, 2017 •
edited
Loading

I don't know why enumerator(at:includingPropertiesForKeys:options:errorHandler:) caused crash but the crash resolved by using contentsOfDirectory(at:includingPropertiesForKeys:options:) instead of it.
Anyway, the latter is simpler, I think.

I thought these problems are involved, but it seems to be other problems.

@mono0926 mono0926 mentioned this pull request Mar 9, 2017
Closed
@codecov-io
Copy link

codecov-io commented Mar 9, 2017 •
edited
Loading

Codecov Report

Merging #620 into master will decrease coverage by -0.04%.
The diff coverage is 65.21%.

@@            Coverage Diff             @@
##           master     #620      +/-   ##
==========================================
- Coverage   75.67%   75.63%   -0.04%     
==========================================
  Files          21       21              
  Lines        2269     2266       -3     
==========================================
- Hits         1717     1714       -3     
  Misses        552      552

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b6e4ae...20cf654. Read the comment docs.

onevcat
onevcat reviewed Mar 9, 2017
View reviewed changes
Sources/ImageCache.swift Outdated
if !onlyForCacheSize {
cachedFiles[fileUrl] = resourceValues
}
for fileUrl in (try? fileManager.contentsOfDirectory(at: diskCacheURL, includingPropertiesForKeys: Array(resourceKeys), options: FileManager.DirectoryEnumerationOptions.skipsHiddenFiles)) ?? [] {
Copy link
Owner

Choose a reason for hiding this comment

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

Sorry for the original bad code, but FileManager.DirectoryEnumerationOptions.skipsHiddenFiles could be simplified to .skipsHiddenFiles.

Copy link
Contributor Author

@mono0926 mono0926 Mar 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@onevcat

Okay, I corrected and amended: 33b3f28

onevcat
onevcat reviewed Mar 9, 2017
View reviewed changes
Copy link
Owner

@onevcat onevcat left a comment

Choose a reason for hiding this comment

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

LGTM. Could you simplify the options before we could merge it? Thanks!

@mono0926
Copy link
Contributor Author

mono0926 commented Mar 9, 2017

@onevcat

Thanks, if it is okay, merge please :octocat:

@onevcat
Copy link
Owner

onevcat commented Mar 9, 2017

Great!

@onevcat onevcat merged commit 4260203 into onevcat:master Mar 9, 2017
@mono0926 mono0926 deleted the fix-background-crash branch March 9, 2017 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@onevcat onevcat onevcat left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mono0926 @codecov-io @onevcat