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

Export file-writing helpers #389

Merged
merged 10 commits into from Jun 20, 2018
Merged

Export file-writing helpers #389

merged 10 commits into from Jun 20, 2018

Conversation

@jennybc
Copy link
Member

@jennybc jennybc commented Jun 19, 2018

Closes #366 any chance you'd consider exporting write_union?
Closes #344 Export edit_file

  • Re-locate some functions. helper.R had gotten quite large and diffuse.
  • Make is_in_proj() resilient to there being no active project
  • Export edit_file(), write_union(), write_over(); mark with @keywords internal
  • Process input path with user_path_prep() now that it's user-accessible
@jennybc jennybc requested a review from hadley as a code owner Jun 19, 2018
@jennybc jennybc removed the request for review from hadley Jun 19, 2018
@jennybc
Copy link
Member Author

@jennybc jennybc commented Jun 19, 2018

Might be nice to add "Export edit_file" #344 to this PR, yes?

R/write.R Outdated
#' @param new_lines Character vector of lines to add, if not already present.
#' @param quiet Logical. Whether to message about what is happening.
#'
#' @return Logical indicating whether a write occurred, invisibly.

This comment has been minimized.

@hadley

hadley Jun 19, 2018
Member

@keywords internal ?

@jennybc jennybc force-pushed the export-write-union branch from b761e65 to 145f9a0 Jun 19, 2018
@jennybc
Copy link
Member Author

@jennybc jennybc commented Jun 19, 2018

Now includes #344 (Export edit_file) + a lot of moving functions around that I have long wanted to do.

R/write.R Outdated
#' readLines(tmp)
#'
#' ## clean up
#' file.remove(tmp)

This comment has been minimized.

@hadley

hadley Jun 19, 2018
Member

I wonder if there's a non-awkward way to give this a name starting with edit_? That would make it more clearly a member of an existing family.

This comment has been minimized.

@jennybc

jennybc Jun 19, 2018
Author Member

Yeah. But perhaps write_over() and write_utf8() should also be exported -- why export only write_union()? And then they make their own little family.

This comment has been minimized.

@hadley

hadley Jun 19, 2018
Member

write_utf8 definitely belongs elsewhere but seems good to export write_over

#' }
edit_file <- function(path) {
path <- user_path_prep(path)
dir_create(path_dir(path), recursive = TRUE)

This comment has been minimized.

@hadley

hadley Jun 19, 2018
Member

Should this call use_directory()? The additional messaging seems like it would be nice.

This comment has been minimized.

@jennybc

jennybc Jun 19, 2018
Author Member

Hmm, I'm a bit boxed into a corner atm. use_directory() assumes that input path is relative to the active project. Whereas all the write_*() helpers and edit_file() assume the input path can be taken at face value.

I'll take this as evidence that the functions re: directory creation aren't quite right yet. Will work on that.

This comment has been minimized.

@hadley

hadley Jun 19, 2018
Member

Oh good point; can definitely happen after this PR.

This comment has been minimized.

@jennybc

jennybc Jun 19, 2018
Author Member

Will sort this out here: #393

@jennybc jennybc force-pushed the export-write-union branch from 10fd44d to 32cd262 Jun 19, 2018
jennybc added 2 commits Jun 19, 2018
@jennybc
Copy link
Member Author

@jennybc jennybc commented Jun 19, 2018

Assuming CI goes well, I think this is done.

@jennybc jennybc changed the title Export write_union() Export file-writing and -editing helpers Jun 19, 2018
@jennybc jennybc changed the title Export file-writing and -editing helpers Export file-writing helpers Jun 19, 2018
@hadley
hadley approved these changes Jun 20, 2018
@jennybc jennybc merged commit 979a865 into master Jun 20, 2018
3 of 4 checks passed
3 of 4 checks passed
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@cboettig
Copy link
Contributor

@cboettig cboettig commented Jun 20, 2018

🎉

@jennybc jennybc deleted the export-write-union branch Jun 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants