PhoenicisOrg / scripts

Phoenicis scripts
GNU Lesser General Public License v3.0
64 stars 49 forks source link

"Resource" vs "Downloader" #1188

Open madoar opened 4 years ago

madoar commented 4 years ago

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 https://github.com/PhoenicisOrg/scripts/issues/1181#issuecomment-570917493

plata commented 4 years ago

@qparis you will need to clarify this.

qparis commented 4 years ago

Resource uses Downloader ; it is more abstract.

Resources manages:

Resources are aimed to be common accross scripts

plata commented 4 years ago

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?

madoar commented 4 years ago

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.

qparis commented 4 years ago

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

madoar commented 4 years ago

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.

qparis commented 4 years ago

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

madoar commented 4 years ago

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.

plata commented 4 years ago

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.

madoar commented 4 years ago

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

plata commented 4 years ago

Yes, we could also extend the Downloader.

madoar commented 4 years ago

Any suggestions what methods an extended Downloader class should support? I could imagine:

plata commented 4 years ago

What's the difference of cached compared to onlyIfUpdateAvailable?

madoar commented 4 years ago

There is none. I didn't look at the concrete implementation I only tried to list some functionality that needs to be supported.

plata commented 4 years ago

I see.