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

Made function factories to recreate the auth and deauth functions in … #58

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
2 participants
@jonthegeek
Copy link

commented Apr 6, 2019

…client packages. Needs to be tested in context of other packages before I'm 100% confident it works, but it seems to work in principle. The environment of the resulting auth function is the only thing I'm not 100% certain I nailed. Closes #57.

@jennybc This is what I mean. What do you think? It takes some of the copy/paste instability out of making auth/deauth functions in client packages.

Made function factories to recreate the auth and deauth functions in …
…client packages. Needs to be tested in context of other packages before I'm 100% confident it works, but it seems to work in principle. The environment of the resulting auth function is the only thing I'm not 100% certain I nailed. Closes #57.
@jennybc

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

I can't review in depth now but I will be working on gargle again soon. But fwiw:

The environment of the resulting auth function

and simplicity / maintainability is why my instinct was to not go down this road.

@jonthegeek

This comment has been minimized.

Copy link
Author

commented Apr 6, 2019

I can understand that instinct... but I'm pretty certain this will work as I expect, and, if so, it makes gargle way easier to implement. After my R4DS office hour I'll test it out in a package and make sure it works as expected.

@jennybc

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

Note that the user of the wrapper package needs to be able to specify scopes.

@jonthegeek

This comment has been minimized.

Copy link
Author

commented Apr 6, 2019

The user can specify! The returned function has a scopes argument, which defaults to what the package creator sets, but can be changed by the user.

It looks like R 3.1 doesn't like something I used in this, so that part is unfortunate. I'll see if I can track it down. Edit: That's not related to this change.

@jonthegeek

This comment has been minimized.

Copy link
Author

commented Apr 6, 2019

I forked googlesheets4 and replaced the sheets_auth definition with this:
sheets_auth <- gargle::make_auth( pkg_name = "googlesheets4", scopes = "https://www.googleapis.com/auth/spreadsheets", error_message = paste( "Can't get Google credentials.\n", "Are you running googlesheets4 in a non-interactive session? Consider:\n", " * sheets_deauth() to prevent the attempt to get credentials.\n", " * Call sheets_auth() directly with all necessary specifics.\n" ) )

Before and after I did that, 1 test failed; the rest all passed. As far as I can tell, it generates exactly the same function.

I understand if you end up rejecting this PR, but I really feel like it makes it easier for users to implement gargle.

@jennybc

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

OK I'll just have to think about it.

This is something else to consider:

We try to avoid creating install or build time dependencies between package A and the version of package B. This has caused problems in the past, when package B updates but package A does not. So you really want package A to be calling into package B, not calling a function defined using the version of package B installed when package A was installed. This is, for example, why devtools exposes functions in, e.g., usethis and pkgload the way it does. I didn't explain that very well, but maybe you can figure out what I'm saying.

@jonthegeek

This comment has been minimized.

Copy link
Author

commented Apr 6, 2019

I could be wrong, but this feels like a special case. And to be clear, it isn't loading the function from gargle; the function from gargle returns a function tailored to the package via the arguments to the gargle function. I put the unique googledrive and googlesheets4 auth functions in the tests as examples to hopefully make it clear.

I was right to worry about the environment of the created functions. …
…This properly places them into the package where they're created.
@jonthegeek

This comment has been minimized.

Copy link
Author

commented Apr 7, 2019

An alternative that might feel less scary/wrong: What about a usethis-like function that writes auth.R (or {yourpackage}_auth.R) in your package's R folder, using arguments to the function to fill in all the things that vary package-to-package as much as possible? That file is so similar package to package, it'd be nice to have something to help set it up. The arguments would be ~the things in the gargle_lookup_table. Possibly then it could/would write all the help documentation without arguments to gargle, making it easier to customize from there? And maybe include a comment in there about how to rebuild it (ie, the original call)?

@jennybc

This comment has been minimized.

Copy link
Member

commented Apr 7, 2019

Yes, a usethis-like templating function is already on my radar. But first I'm gaining more experience by wiring gargle into several packages to see how things work.

@jennybc

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

Thanks for exploring this idea. It is appealing conceptually, for sure.

But I'm sticking with the current approach that's based more on templating and documentation, with full acknowledgement that it's not terribly elegant.

I am somewhat concerned with too much abstraction and maintainability.

But the main reason we didn't walk down this road is that this approach needs even more indirection to make it safe. We don't want to create build-time dependencies between gargle and client packages that may use it. Here are some links about this pretty subtle, insidious gotcha:

https://r6.r-lib.org/articles/Portable.html#potential-pitfalls-with-cross-package-inheritance

hadley/r-pkgs#550

r-lib/devtools#1788

@jonthegeek

This comment has been minimized.

Copy link
Author

commented Apr 30, 2019

No problem! This exploration led me down a road that has me building a package for more efficient function-factory creation (https://github.com/jonthegeek/factory), so I'll be sure to take those caveats to heart!

@jonthegeek jonthegeek deleted the jonthegeek:auth-factories branch Apr 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.