nathanielwarner / flightgear-photoscenery

FlightGear - PhotoScenery Edition
GNU General Public License v2.0
19 stars 6 forks source link

Store fetched tiles in a cache; remove them when the bucket is assembled #15

Closed frougon closed 1 year ago

frougon commented 1 year ago

Hi Nathaniel,

I've only started to use your photoscenery stuff very recently; the results look great in the countryside! I believe the commit in this pull request would be a useful addition: it adds caching so as to avoid repeated downloads of the same tiles when a server or network error causes the script to abort before all tiles for the specified bucket have been downloaded. You'll find more details in the commit message.

If you merge this commit, I could file another pull request that would implement the following option:

     parser.add_argument('--clear-cache', metavar='PROVIDER', help="""\
Clear the tile cache directory (for creator.py) associated to PROVIDER.
Specify 'ALL' in order to clear the tile cache directories of all providers.
Only use this option if you really, really know what you are doing, otherwise
you are likely to download the same files several times from PROVIDER.""")

Note that I spelled the options --cache-dir and --clear-cache , not --cache_dir resp. --clear_cache. The former looks much more common in the GNU, Unix and Linux worlds at least, and less ugly IMHO. :) But as you are the author, I can rename these options if you really like the underscore.

Thanks!

nathanielwarner commented 1 year ago

Thanks for this! I did change cache-dir to cache_dir for consistency with the other command line options. (And please do if you follow up with the clear cache capability.) I agree it's ugly and doesn't follow the Unix convention, but I think it would be worse to introduce a different scheme for some of the options.

frougon commented 1 year ago

Thanks for the merge!

I understand your consistency concern and thus the option name change. Another possibility would be to use --usual-convention everywhere but maintain backward-compatibility aliases for the currently-existing options using --other_convention. I haven't tested it, but declaring an option alias with argparse seems very simple:

https://stackoverflow.com/a/52994694

nathanielwarner commented 1 year ago

That does seem to work! I wasn't aware you could do that with argparse. I've added the ability to use dashes for each of the multi-word arguments.

frougon commented 1 year ago

Great, thank you! :-)