agrc / palletjack

A library for updating AGOL data from various external sources
https://agrc.github.io/palletjack/palletjack/
MIT License
12 stars 0 forks source link

Google Drive downloader: Create separate classes for authenticated/nonauthenticated downloads #23

Open jacobdadams opened 1 year ago

jacobdadams commented 1 year ago

suggestion: I noticed another loader class takes the credentials in the ctor and creates a class client. It's nice to have consistency between classes for the sake of the api.

_Originally posted by @steveoh in https://github.com/agrc/palletjack/pull/21#discussion_r963984025_

steveoh commented 1 year ago

Separate classes for auth/anon may not have been my intention from that comment. However, it may be an improvement if you like the idea.

I noticed one class takes the creds as a ctor parameter and creates a client used within the instance and the other class takes credentials as a method parameter and creates the client in the method. That was the inconsistency I wanted to highlight.

jacobdadams commented 1 year ago

Sorry, yeah, I was taking it one step further without being clear 🙂

Because that class has methods for downloading both with authentication and without, I intentionally didn't put the creds in the constructor. However, you're totally right, that is inconsistent with the other class in the module. I'm thinking the best fix is to split the class into two classes, one for auth'ed downloads and another for anon. I think that will give the best ux.

(Or honestly, it may make sense to just remove the anon downloader, since google's abuse-blocking lockout makes using it fragile and finicky. But it may be nice to keep it for small things, or keep and modify it for situations when sharing the files with the service account is not feasible. Just thinking out loud here.)