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

Updated error handling for do_download function #26

Merged
merged 3 commits into from
Mar 15, 2015

Conversation

amr66
Copy link
Contributor

@amr66 amr66 commented Mar 12, 2015

As discussed here: #12
This is my approach...

maphew added a commit that referenced this pull request Mar 15, 2015
Updated error handling for do_download function
@maphew maphew merged commit 469ee44 into maphew:master Mar 15, 2015
@maphew
Copy link
Owner

maphew commented Mar 15, 2015

Thank you very much for the contribution Andreas! It is certainly better than we what have now.

and yet, I hesitate. I'm uncomfortable with the pattern of returning number codes, because it means a lookup table must be kept in some other location. If I say do_thing(there) and it tells me -3 in return I need to go somewhere else to translate -3 into meaning, "sorry, nothing to do, 'there' doesn't exist".

As you note in other comment, raising an exception would be better. I'll follow up more there when I've had a chance to think through things more, and experiment. Accepting the patch for now, because it is an improvement, but will resist replicating the pattern elsewhere pending the experiments.

@maphew
Copy link
Owner

maphew commented Mar 15, 2015

I'd like to understand better the problem you ran into, that returning error codes solves for you. I've been doing a lot of reading and becoming more convinced a better long term resolution involves returning exceptions. How are you using apt? Can you describe the scenario that creates the problem? (hopefully repeatable)

@maphew
Copy link
Owner

maphew commented Mar 15, 2015

I just committed 16422c3, an attempt to exception and raise in a way that might be useful for callers.

@amr66 amr66 deleted the amr66-patch-1 branch March 17, 2015 09:04
@amr66
Copy link
Contributor Author

amr66 commented Mar 17, 2015

Congratulations, i took a short view yesterday. I've learned something new about raise as you use it to "reraise" an Error.

I use apt.py as a module and wrote my own postinstall function to install osgeo4w packages. My "own" apt has a main function, similar to your global stuff. From there i set the needed variables, that are global in apt.py.
Just a short sketch:

import apt
# ...
apt.root = my_special_value
# ...
def install(packages): # my very own install function
    # ...
    for p in required:
        apt.do_download(p)
        postinstall(p)  # my version of postinstall
#... and so on

Maybe i create a git project for it?
I had some problems downloading packages so my awareness increased on that function and a better handling. If a download fails, what is to do next? Retry? If you have multiple required packages and only some fail? Delete them?
I will anotate in your code?

@maphew
Copy link
Owner

maphew commented Mar 20, 2015

ok, thanks for the example. I started a similar experiment (.\wildlands\a2.py) but abandoned when I realized I needed to fix many more things within apt itself to make it workable. (Also helped by understanding that Plac introduced more complexity than I was ready to to deal with.)

If your work was on Github, or any other public resource, it would make it easier to see and test the effect my changes make.

Failed download: 1 or 2 automatic retries would make sense. Though I haven't run into a situation yet where that would make a difference. So far, for me, every time there's been a failed download it's needed another response (the file doesn't exist, download.osgeo.org is down, ran out of disk space in the cache volume).

Failed requires downloads: A cache cleaner might be a good future enhancement: go through cache and delete old versions, and any packages that aren't installed. To begin with I'd probably just add a function remove named packages though, something like apt cache-delete shell iconv. I don't think I'd automatically remove incomplete requires downloads; it would make a retry expensive (re-downloading what already worked in attempt to get the one package that failed).

Annotation is welcome. If something wasn't clear to you, it probably is confusing to others (and probably me too). ;-)

More generally, to the idea of using apt as a module: the project is definitely still alpha, with many internals up for significant change, as I make the weak internal architecture stronger. I'm happy for people to use apt, just be aware it's an unstable, moving, evolving project.

maphew added a commit that referenced this pull request Mar 21, 2015
The progress indicator is now inline, but it would be better to have it
reusable. Still TODO is refactor/update down_stat() or otherwise improve
this user feedback.
@maphew maphew mentioned this pull request Mar 22, 2015
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.

2 participants