@dblock => Allow a Fog::Storage uploader to be passed in for uploading of generated tiles rather than local storage #5
Conversation
|
This is pretty hacky. Lets use some OO? I'd like you to extract a class that implements the same interface to store files and make an implementation for local files and another for S3. So instead of You also need to update CHANGELOG and possibly README if you're going to expose this in the command line tool. |
| @tile_quality = options[:quality] || DEFAULT_QUALITY | ||
| @overwrite = options[:overwrite] || false | ||
| @destination = options[:destination] || File.join(Dir.pwd, "tiles") | ||
| @uploader = options[:uploader] |
There was a problem hiding this comment.
This should be @storage = options[:storage] || FileStorage.new(@destination).
mzikherman
force-pushed
the
uploader
branch
2 times, most recently
from
f1812eb
to
16bb574
Compare
| FileUtils.mkdir_p(current_level_dir) | ||
|
|
||
| current_level_storage_dir = @storage.storage_location(level) | ||
| @storage.mkdir(current_level_storage_dir) |
There was a problem hiding this comment.
no-op since S3 doesn't have directories, no need to create anything ahead of time, as long as the right key/prefix is used.
|
Updated @dblock . Thanks for the comment on making it more OO rather than just modifying the existing utility. I moved the logic of constructing the directory/filename of resultant files and storage of the files into the new classes, it definitely feels cleaner now. |
| raise "Output directory #{@destination} already exists!" | ||
| @overwrite ? Rails.logger.warn(msg) : raise(msg) | ||
| end | ||
| @storage.handle_overwrite_existing_files! |
There was a problem hiding this comment.
This is something I would not sink into storage, the interface is basically "does the destination exist?", and if it doesn't, fail - it's weird that the failure is in the file storage itself.
The old condition body looks like a bug, and a bad copy-paste from some old code too - there's no such thing as msg, much less Rails.logger :)
raise "Output #{@destination} already exists!" if !@overwrite && storage.exists?There was a problem hiding this comment.
That's what I thought! I was reading it like wtf, but kept it :(
|
This is 99% there, see my comments. Add a line to CHANGELOG too, please (create a "Next" section). |
|
Amended once more, thanks again for the feedback. |
| @@ -1,3 +1,7 @@ | |||
| ### Next | |||
|
|
|||
| * [#5](https://github.com/dblock/dzt/pull/5) Allow a Fog::Storage uploader to be passed in for uploading of generated tiles rather than local storage | |||
There was a problem hiding this comment.
This is missing a link to you, - @.... See lines below.
|
Updated. |
…ted tiles rather than local storage
@dblock => Allow a Fog::Storage uploader to be passed in for uploading of generated tiles rather than local storage
Will use this library with Gemini.