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

Do cleanup if there is an error mid-download #68

Merged
merged 6 commits into from
Sep 12, 2018
Merged

Conversation

oxinabox
Copy link
Owner

Resolves #66 (comment)

This can't be merged until JuliaLang/METADATA.jl#17037
Because it needed new version of ExpectationStubs.jl for tests

A version that doesn't require that will be back-ported to 0.6 see #64

@codecov-io
Copy link

codecov-io commented Aug 19, 2018

Codecov Report

Merging #68 into master will increase coverage by 0.48%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #68      +/-   ##
==========================================
+ Coverage   81.75%   82.23%   +0.48%     
==========================================
  Files          11       11              
  Lines         148      152       +4     
==========================================
+ Hits          121      125       +4     
  Misses         27       27
Impacted Files Coverage Δ
src/fetch_helpers.jl 100% <100%> (ø) ⬆️
src/resolution_automatic.jl 69.69% <100%> (+4.17%) ⬆️

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 115618f...06e9879. Read the comment docs.

@oxinabox
Copy link
Owner Author

@Evizero can I borrow a code review?
This should be fairly simple and short.

test/main.jl Outdated


@show dummydown
@show dummyhash
Copy link
Collaborator

Choose a reason for hiding this comment

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

these look like leftover debug statements

Copy link
Owner Author

Choose a reason for hiding this comment

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

Indeed.

test/main.jl Outdated
register(DataDep("TestErrorChecksum", "dummy message", "http://example.void",
(error, "1234"); # this will throw an error
fetch_method=dummydown))
@test_throws Exception datadep"TestErrorChecksum"
Copy link
Collaborator

Choose a reason for hiding this comment

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

not super important, but you could use an ErrorException here to be just a tiny bit more confident that you are catching the error you expect to catch

Copy link
Owner Author

Choose a reason for hiding this comment

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

Good point. I remember thinking that. I don't know why I didn't

@Evizero
Copy link
Collaborator

Evizero commented Aug 20, 2018

Can you think of a situation where you would not automatically want to remove the localdir on error? For example, what if the folder exists already?

@oxinabox
Copy link
Owner Author

oxinabox commented Aug 20, 2018

Can you think of a situation where you would not automatically want to remove the localdir on error? For example, what if the folder exists already?

Yeah, I was wondering about that.
In theory this can't occur, unless the download is triggered dirrectly.
since resolve will not do a download, if there is already a folder with that name.
Possibly though download should do a sanity check outside the try-catch saying:
something like

Folder already exists and is non-empty. What would you like to do?

@oxinabox oxinabox merged commit 1cb28fa into master Sep 12, 2018
@oxinabox oxinabox deleted the ox/errorsafe branch November 8, 2018 14:38
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