-
Notifications
You must be signed in to change notification settings - Fork 79
Check for path errors eagerly during load/save (#263) #265
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
Conversation
|
By the way, is |
Codecov Report
@@ Coverage Diff @@
## master #265 +/- ##
==========================================
+ Coverage 76.02% 76.31% +0.28%
==========================================
Files 8 8
Lines 413 418 +5
==========================================
+ Hits 314 319 +5
Misses 99 99
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ianshmean Thanks! I've recently seen a lot of people bit by this obscure error message, even for core devs in JuliaImages. For example, JuliaImages/Images.jl#888 JuliaImages/ImageEdgeDetection.jl#7
One reason I didn't put my hands on this is that I feel it over-eager to check whether the directory exists when saving images in FileIO; I feel that should depend on backends implementations. The other two checks look good to me.
src/loadsave.jl
Outdated
| unknown(q) && throw(UnknownFormat(q)) | ||
| if q isa File | ||
| isdir(filename(q)) && throw(ArgumentError("Given file path is a directory: $(filename(q))")) | ||
| !isdir(dirname(filename(q))) && throw(ArgumentError("Directory of file path does not exist: $(filename(q))")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My best guess is that some backends might support path creation before writing, so checking if the directory exists is perhaps too eagerly for a general IO frontend.
Checking whether the file path is a directory looks good to me.
|
Perhaps a solution could be to always |
|
Not sure how others think, mkpath by default sounds a good solution to me |
I would also like this to be the default. |
|
Ok, great. I've made that the case now |
johnnychen94
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Should we wait for more reviews? |
|
I think it's possible we'll be troubled by the path-creation, but this is undeniably better and might be perfect. Let's try it and see how we like it. I think this merits a minor version bump, though? As a new feature? (That won't be breaking as we're over 1.0.) |
|
Minor bump sounds right. Thanks! |
|
Thanks for fixing this, BTW! |
This adds more eager checking for path errors during load & save for clearer error messages.
Fixes #263
Loading an a nonexistant file
Before:
This PR:
Trying to save into a directory that doesn't exist
Before:
This PR:
UPDATE: Now
mkpath()is run on the parent directory, if it doesn't exist and no error is thrownTrying to save to a path that's a directory
Before
This PR:
cc. @johnnychen94 @timholy