r-lib / gitcreds

Query git credentials from R
https://gitcreds.r-lib.org/
Other
27 stars 10 forks source link

Notes from early usage #5

Closed jennybc closed 3 years ago

jennybc commented 4 years ago

I'm going to plumb this into usethis experimentally and will take a few notes here as I go.

If this is going to be a "drop in" file (I think that's what #3 is about), some of the function names are problematic:

Two other functions that seem ripe for collision (but that aren't currently a problem for me, that I know of):

gaborcsardi commented 4 years ago

It will be all in a big local({ }) block, with only the public API "exported" from it.

jennybc commented 4 years ago

I am using the "big local({ }) block" approach in an experimental branch of usethis. In general, yes this eliminates the name collision problems I was worried about.

(I found and replaced two calls to base::interactive() with is_interactive().)

But gitcreds's is_interactive() still eventually calls base::interactive(). And in usethis I have a shim to keep base::interactive() use from creeping in:

https://github.com/r-lib/usethis/blob/77d96f89fd22b1a7d1af5a3f6873eb8943ed32bb/R/utils.R#L60-L65

So then I see this:

> gitcreds$gitcreds_set()
Error: Internal error: use rlang's `is_interactive()` instead of `base::interactive()`
Run `rlang::last_error()` to see where the error occurred.
> le
<error/rlang_error>
Internal error: use rlang's `is_interactive()` instead of `base::interactive()`
Backtrace:
 1. gitcreds$gitcreds_set()
 2. usethis::is_interactive() /Users/jenny/rrr/usethis/R/gitcreds.R:37:2
 3. usethis::interactive() /Users/jenny/rrr/usethis/R/gitcreds.R:554:4
 4. usethis::ui_stop("Internal error: use rlang's {ui_code('is_interactive()')} \\\n     instead of {ui_code('base::interactive()')}") /Users/jenny/rrr/usethis/R/utils.R:61:2

I know we don't plan to inline gitcreds into usethis, long-term, so perhaps this is not worth worrying about.

jennybc commented 4 years ago

Something about this prompt is off:

Screen Shot 2020-09-13 at 6 05 08 PM

(referring to the cursor blinking on line i but the text prompt appearing on line i + 1)

jennybc commented 4 years ago

When we fix 👆, shall we also use some approach to obscuring the pasted-in PAT?

gaborcsardi commented 4 years ago

That seems like an RStudio issue:

readline("\nfoobar? ")

will show as

> readline("\nfoobar? ")
        yeah
foobar? 

is Rstudio. But it is possible to work around it.

When we fix 👆, shall we also use some approach to obscuring the pasted-in PAT?

I don't think that it is needed, the PAT is most likely copy pasted from a browser window, and it is not obscured there, either.

jennybc commented 4 years ago

the PAT is most likely copy pasted from a browser window, and it is not obscured there, either.

Well, unless you get a new PAT every time one gets (intentionally or accidentally) deleted, you presumably have some other record of it. I keep mine in 1Password and recommend that others do the same (and by default in 1Password a field like this is obscured).

I'm not fixated on this (and yet I'm also 100% sure we will get this feedback).

gaborcsardi commented 3 years ago

I'll close this now, but please open separate issues if you still have concerns with some of these issues.