"Resource" vs "Downloader"
madoar opened this issue · 15 comments
Currently we are using a mix of Resource and Downloader in our scripts. Both classes/modules do similar if not the same tasks, they allow downloading file(s) from the internet to a given folder/file.
My question is now which of the two classes should we recommend/advocate for the script usage?
See also my arguments in #1181 (comment)
Resource uses Downloader ; it is more abstract.
Resources manages:
- Downloading an existing file into a resource bucket
- Verifying if the resource already exists before doing it
Resources are aimed to be common accross scripts
Resources are aimed to be common across scripts
Ok, this was my understanding as well. So can we generalize this to the rough rule to use Resource mainly in verbs but not when downloading app installers/patches?
So can we generalize this to the rough rule to use Resource mainly in verbs but not when downloading app installers/patches?
Why not use Resource everywhere? In my opinion the usage of Resource is easier and more comfortable especially when you use it download installers or archives that are deleted afterwards anyway.
Because in many cases, you don't want to cache the result. For example, a 2Go game, etc...
The idea is to anticipate the cases where the file will be downloaded more than once. I think verb=Resource and script=Downloader is indeed a good start
Because in many cases, you don't want to cache the result.
Strictly speaking Resource does not cache the downloaded file. It only checks whether it already exists, in which case it is not downloaded again. This is not a dedicated caching approach where the file is intentionally retained. The same as when using Downloader the downloaded file must be manually removed if it should be deleted.
The only disadvantage Resource has compared to Downloader is in my opinion its name, i.e. Resource is a bit generic in comparison to what it actually does, i.e. download something.
If the fact that Resource saves files to "application.user.resources" we could extend it with a destination(...) method that allows the specification of the target directory for the downloaded.
This is indeed exactly the point. Resource is more abstract : it should be used when a reusable resource is downloaded.
Downloader is a lower level and less abstract verb. In many cases, a script developer should not care about where the file is stored. It is not a useful information and should be hidden from them
In many cases, a script developer should not care about where the file is stored.
That is my main issue with Downloader. Downloader requires the script developer to specify to(localDestination), otherwise it does not know where to store the file. Resource does not need that much information, it is fine with a filename.
Another benefit of Resource compared to Downloader is that its usage is much easier. When you use Downloader you need to remember where the file has been downloaded to and how it is called. Resource in comparison returns the path leading to the downloaded file. This is much more comfortable and less error prone because the script developer does not need to create the path himself.
What about:
Add a new util on top of Downloader (e.g. InstallerDownloader) which uses createTmpFile() to download to a file which will be deleted when Phoenicis is closed.
This sounds like an idea. But why do we need to add a module class for that? Can't we extend our current Resource module or Downloader module to support this as well? Ideally we should be able to specify this using additional builder methods that set the corresponding settings
Yes, we could also extend the Downloader.
Any suggestions what methods an extended Downloader class should support? I could imagine:
.temporary(boolean): only retains the file during script execution.cached(boolean): checks whether the file has been downloaded before.get(): string/.download(): string/...: downloads the file and returns the path leading to the file
What's the difference of cached compared to onlyIfUpdateAvailable?
There is none. I didn't look at the concrete implementation I only tried to list some functionality that needs to be supported.
I see.