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.

Sign up for GitHub

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

Jump to bottom

functions that write to the filesystem #96

Closed
ijlyttle opened this issue Mar 2, 2019 · 6 comments · Fixed by #109
Closed

functions that write to the filesystem #96

ijlyttle opened this issue Mar 2, 2019 · 6 comments · Fixed by #109

Comments

@ijlyttle
Copy link
Member

ijlyttle commented Mar 2, 2019

These need to be looked at with suspicion and scrutiny:

  • box_auth_on_attach() writes to the .Renviron file (and is perhaps more trouble than it's worth)
  • box_auth() writes a file to the working directory (necessarily so)
@nathancday
Copy link
Member

I personally use box_auth_on_attach() in my day-to-day because I am constantly refreshing my Rsession and re-running a script from scratch. That said I don't think this is necessarily needed, my co-workers just use box_auth(). I don't see the harm in removing box_auth_on_attach if we still keep the Renviron special variable and demonstrate how to replicated the behavior by editing the .Renviron directly, similar to install_github() pulling the PAT from .Renviron for private repos.

box_auth() seems like it has to stay as is or am I missing something?

@ijlyttle
Copy link
Member Author

ijlyttle commented Mar 3, 2019 •
edited

Absolutely, box_auth() has to stay!

I guess I am a bit nervous about box_auth_on_attach(), given the notes that @brendan-r made about reproducibility and the newer CRAN rules about writing to the file system (I suspect particularly so for dot files). Whenever we do this, I think we need to be especially clear to the user about what the function will do.

@nathancday
Copy link
Member

I defer to your CRAN knowledge and think it would be okay to remove box_auth_on_attach as an exported function and replace it with a vignette of how to edit .Renviron for similar behavior

@ijlyttle
Copy link
Member Author

ijlyttle commented Mar 3, 2019 •
edited

I think it might be a matter doing something like usethis does: copy the lines to your clipboard, then open the .Renviron file for editing.

@brendan-r
Copy link
Collaborator

brendan-r commented Mar 28, 2019 •
edited

Oof, box_auth_on_attach was always a nasty hack that encouraged 'bad' behavior. I felt unsure when writing it, and sort of wanted to kill it off myself.

If this is causing problems for CRAN submission, I wouldn't feel too bad getting rid of it. AFAICT this shouldn't cause problems for users; the function was more to set preferences (a 'one off') rather than to be invoked as part of a script.

If there's concern that people were using it in-place of box_auth() in analysis scripts, we could just make it call box_auth,with a warning message that use of box_auth_on_attach is no longer recommended, and will be removed in the next release.

@ijlyttle
Copy link
Member Author

ijlyttle commented Mar 28, 2019 •
edited

Perhaps box_auth_on_attach() could invoke warning if it is not used interactively. I suspect that there are some safe ways to implement and keep box_auth_on_attach(), I think we just need to be deliberate about it.

ijlyttle added a commit that referenced this issue Jul 21, 2019
@ijlyttle
Rework box-auth functions (fix #96):
ae97bc5 
- deprecate box_attach_on_auth()
- deprecate write.Renv arg to box_auth()
  - write environment variables to console instead
- refactor box_auth(), box_fresh_auth():
  - import glue and fs (both low depencency)

Also, fix typo in box_read() documentation
@ijlyttle ijlyttle mentioned this issue Jul 21, 2019
Merged
ijlyttle added a commit that referenced this issue Jul 21, 2019
@ijlyttle
Rework box-auth functions (#109)
971144b 
* Rework box-auth functions (fix #96):

- deprecate box_attach_on_auth()
- deprecate write.Renv arg to box_auth()
  - write environment variables to console instead
- refactor box_auth(), box_fresh_auth():
  - import glue and fs (both low depencency)

Also, fix typo in box_read() documentation

* Add deprecation to documentation, add is_void() to internal functions
ijlyttle added a commit that referenced this issue Aug 3, 2019
@ijlyttle
Update documentation (#112)
f85668d 
* Rework box-auth functions (fix #96):

- deprecate box_attach_on_auth()
- deprecate write.Renv arg to box_auth()
  - write environment variables to console instead
- refactor box_auth(), box_fresh_auth():
  - import glue and fs (both low depencency)

Also, fix typo in box_read() documentation

* Add deprecation to documentation, add is_void() to internal functions

* Update documentation for functions:

- box_auth()
- box_ls()
- box_setwd()
- box_dir_create()

* update docs

* Update docs

* Update function-documentation

* Complete first-pass through function-documentation

* clean up function-documentation

* Puts in placeholders for JWT authentication

* Split vignettes

* Update R/boxr_misc.R

Co-Authored-By: Nate <nathancday@gmail.com>

* Update R/boxr_read.R

Co-Authored-By: Nate <nathancday@gmail.com>

* Make S3 class-descriptions consistent

* remove author from function-docs, we will rely on package authorship for attribution

* - Deprecate filename, not file_name
- Harmonize punctuation

* Add `...` arg to box_source(), passed to source().

Update documentation for `source()`.

Harmonize other documentation.

* Clean up the deprecation of `filename` argument

* update hyperlinks to box.com, rebuild vignettes

* Go with a less-is-more approach, let the reader refer to the rio documentation if they wish.

* removed hanging backtick in box_read() and re-doc'd for changed roxygen front-matter

* Clean-up
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Rework box-auth functions r-box/boxr
3 participants
@nathancday @ijlyttle @brendan-r