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

"Resource" vs "Downloader" #1188

Open
madoar opened this issue Jan 5, 2020 · 15 comments
Open

"Resource" vs "Downloader" #1188

madoar opened this issue Jan 5, 2020 · 15 comments
Assignees

Comments

@madoar
Copy link
Collaborator

madoar commented Jan 5, 2020

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)

@plata
Copy link
Collaborator

plata commented Jan 5, 2020

@qparis you will need to clarify this.

@qparis
Copy link
Member

qparis commented Jan 7, 2020

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

@plata
Copy link
Collaborator

plata commented Jan 8, 2020

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
Copy link
Collaborator Author

madoar commented Jan 8, 2020

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
Copy link
Member

qparis commented Jan 8, 2020

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
Copy link
Collaborator Author

madoar commented Jan 9, 2020

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
Copy link
Member

qparis commented Jan 9, 2020

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
Copy link
Collaborator Author

madoar commented Jan 9, 2020

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
Copy link
Collaborator

plata commented Jan 9, 2020

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
Copy link
Collaborator Author

madoar commented Jan 14, 2020

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
Copy link
Collaborator

plata commented Jan 15, 2020

Yes, we could also extend the Downloader.

@madoar
Copy link
Collaborator Author

madoar commented Jan 30, 2020

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

@plata
Copy link
Collaborator

plata commented Jan 30, 2020

What's the difference of cached compared to onlyIfUpdateAvailable?

@madoar
Copy link
Collaborator Author

madoar commented Feb 8, 2020

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

@plata
Copy link
Collaborator

plata commented Feb 15, 2020

I see.

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

No branches or pull requests

3 participants