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

refactor+docs: set_api_key #217

Merged
merged 11 commits into from
Nov 18, 2023
Merged

refactor+docs: set_api_key #217

merged 11 commits into from
Nov 18, 2023

Conversation

dshemetov
Copy link
Contributor

@dshemetov dshemetov commented Nov 16, 2023

closes #181

Changes:

  • get_api_key() only reads from environment variable now, no R options
  • save_api_key() now instructs the user to write their key and dynamically opens ~/.Renviron or proj_root/.Renviron
  • update documentation to reflect this

R/auth.R Outdated Show resolved Hide resolved
Copy link
Contributor

@capnrefsmmat capnrefsmmat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple ideas

R/auth.R Outdated Show resolved Hide resolved
R/auth.R Show resolved Hide resolved
Co-authored-by: Alex Reinhart <alex@refsmmat.com>
@dshemetov
Copy link
Contributor Author

Might be good to go.

Copy link
Contributor

@brookslogan brookslogan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Made a few suggestions. There are still some references to "set" in the text; some definitely should still be "set", but not sure if others should be "save" or something else; I didn't see any place where I was convinced just "save" would be better though.

R/auth.R Outdated
Comment on lines 14 to 18
#' We recommend setting your key with `save_api_key()`, which will modify your
#' `.Renviron` file and will persist your key across R sessions (see `?Startup`
#' for a description of `.Renviron` files). Alternatively, you can modify the
#' environment variable in the shell or with `Sys.setenv()`, but these will not
#' persist across sessions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#' We recommend setting your key with `save_api_key()`, which will modify your
#' `.Renviron` file and will persist your key across R sessions (see `?Startup`
#' for a description of `.Renviron` files). Alternatively, you can modify the
#' environment variable in the shell or with `Sys.setenv()`, but these will not
#' persist across sessions.
#' We recommend setting your key with `save_api_key()`, which will modify an
#' applicable `.Renviron` file, which will be read in automatically when you
#' start future R sessions (see [`?Startup`][base::Startup] for details on
#' `.Renviron` files). Alternatively, you can modify the environment variable at
#' the command line before/while launching R, or inside an R session with
#' [`Sys.setenv()`], but these will not persist across sessions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub's just randomly not highlighting changes for me in some places. This reworded "your", "persist your key across", "for a description of", and "in the shell", plus added links to ?Startup and Sys.setenv().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, ?Startup and Sys.setenv() is auto-linked by roxygen (see here for example).

R/auth.R Outdated Show resolved Hide resolved
R/auth.R Outdated Show resolved Hide resolved
@brookslogan
Copy link
Contributor

brookslogan commented Nov 18, 2023

One thing that would be nice, but I imagine might actually be stupidly tricky, is to first tell the user what file is being modified, before they press enter to confirm.

[EDIT: just saw the conversation about this above. I did not notice the usethis messages about the path before. Maybe okay but I still hope we can move to something that doesn't require this manual editing step eventually.]

dshemetov and others added 3 commits November 17, 2023 16:24
Co-authored-by: brookslogan <lcbrooks@cs.cmu.edu>
Co-authored-by: brookslogan <lcbrooks@cs.cmu.edu>
Co-authored-by: brookslogan <lcbrooks@cs.cmu.edu>
@dshemetov
Copy link
Contributor Author

dshemetov commented Nov 18, 2023

Yea, that'll be hard. I think we found the minimal effort, maximal user gain solution, so we should probably just go with it.

I thought about manually appending to .Renviron with write(paste("DELPHI_EPIDATA_KEY=", key), environ_path, append=TRUE), but got stuck at trying to find the OS-independent user .Renviron path again. rappdirs would only point me to config or cache dirs, but not user environs. There's probably another package that we sifted through in the other thread that has something, but I don't think digging in on this is worth it compared to other things we could work on. Let's wait til we get feedback actual users before optimizing.

@brookslogan
Copy link
Contributor

btw, this document command looks super handy with GitHub suggestions. Very cool. (And doesn't seem like there should be conflicts if you need to rebase local changes made in parallel to the doc commits.)

@dshemetov
Copy link
Contributor Author

dshemetov commented Nov 18, 2023

Right? It works well enough. (Wonder if we can have CI trigger from its commits too, because sometimes people push without documenting NAMESPACE changes, the CI breaks, but then the CI document pushes a change, but the status of the CI doesn't change to passing. But tuning GH CI often feels like another "pencil straightening" exercise to me.)

@dshemetov dshemetov merged commit 9a7e505 into dev Nov 18, 2023
@dshemetov dshemetov deleted the ds/api-keys branch November 18, 2023 00:34
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

Successfully merging this pull request may close these issues.

Figure out better API key management
3 participants