-
Notifications
You must be signed in to change notification settings - Fork 5
-
Notifications
You must be signed in to change notification settings - Fork 5
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
Feature request: merge with Credentials package #12
Comments
To clarify, I see |
|
Thanks for contacting us. I'm definitely not opposed to working together, although a separate package for comprehensively handling user credentials could easily make sense as well. One that, in addition to your additions, automatically handled hashing/authenticating could be useful, for example. At some point I convinced myself that rstudioapi had to be a hard import. I'll take another look though. |
@snoweye those are both in I don't have a definitive answer on the license issue at this time. I'd have to look into it more but iirc both licenses are pretty permissive. @wrathematics we discussed leaving it as a separate package but think the principal downside is that bug reports might get filed in the wrong place. since we didn't foresee changing the existing getPass public API, it seemed like adding functionality to an already existing package was slightly preferable. If you are willing to export the individual acquisition functions, we can explore this route. Ultimately either solution is fine with us, but we wanted to discuss with you all to find the best solution for the R community. :) |
You don't need testthat for tests or knitr for vignettes though. We both feel pretty negatively about those packages (I have very strong negative feelings about testthat in particular). This package currently has a latex vignette, although we used to use knitr. Dropping knitr halved the time spent checking the package on travis, for example. |
An extension/advance package on top of either or both packages may be easier for both. Unless these can get very popular, don't need to worry about wrong bug reports ... |
To make that work we would still need to modify the namespace here so that credentials could depend on it. I'm awaiting guidance about the licensing issue. Expect to hear back early next week (Tuesday or Wednesday). I agree that the bug report issue is likely not going to be a big deal, but I would still like to avoid entirely duplicating what's already been released. |
|
Thanks to a lot of legwork by my co-conspirator @mattpollock, he was able to get a number of R packages publicly released by MITRE. One of those packages is
credentials
which we started beforegetPass
was available on CRAN. There is a lot of duplicated functionality between the two so rather than pushingcredentials
to CRAN, we were wondering if you would be open to a PR (or series of) that added some functions fromcredentials
.The biggest difference is that the main functions in credentials provide a list with both the username and the password in the return value. There is also a
shiny + miniUI
variant of username/password acquisition. The other difference is thatcredentials
exposes the functions for each acquisition method rather than the autodetect method used bygetPass
.What we would like to do:
getCredentials()
public function which returns a list as described above.guessUser()
public helper function.shiny + miniUI
acquisition methodrstudioapi
toSUGGESTS
(we think you only use this in one file and it would be a small change)I'm happy to discuss any details of this or come up with alternatives if you are interested.
The text was updated successfully, but these errors were encountered: