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

Patch 142 #145

Merged
merged 8 commits into from
Oct 3, 2017
Merged

Patch 142 #145

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bda54c0
This will temporarily alleviate #142
karthik Oct 2, 2017
ee251da
Merge branch 'master' into patch-142
karthik Oct 2, 2017
9955d04
Replaced drop_dir with a direct call to drop_list_media
karthik Oct 3, 2017
5b79e4c
Bumped version
karthik Oct 3, 2017
2f96bf1
Removed old messages
karthik Oct 3, 2017
4d1bd8c
Returned version num to 0.8.1.9999
karthik Oct 3, 2017
7197a9d
Added a small test for subfolders
karthik Oct 3, 2017
8c97e8c
Adding an explicit test for #142
karthik Oct 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion DESCRIPTION
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
Package: rdrop2
Title: Programmatic Interface to the 'Dropbox' API
Version: 0.8.1.9999
Version: 0.8.2.9999
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need to bump the version here, do we? this implies 0.8.2 already exists, but it doesn't. I think the point of .9999 is to say "caution, this stuff is in flux" and avoid some complicated gitflow-like workflow with release branches and all that

Copy link
Owner Author

Choose a reason for hiding this comment

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

Because the PR is a patch? My rationale: If someone files a similar issue but have not updated to this change, we cannot tell if they have this fixed version or the last cran release. We could see the hash if they give us devtools::session_info but not otherwise.

What would you suggest as a workflow for bumping minor? Just with each release?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good point. Looking at other packages (dplyr, purrr, httr) looks like they only go from "real" 3-number releases to 3-number-plus-9000 (not 9999) and back again, keeping the .9000 version for some time.

I get wanting to tell where users are when they file issues, but bumping the version everytime we push to master seems like too much; we might quickly be at 8.2.99.9999, and it invites a plethora of merge conflicts since every PR will change that line.

I definitely lean towards leaving the version until we're ready for a CRAN release or switching back to development, but we don't have to do that.

Authors@R: c(person("Karthik", "Ram", email = "karthik.ram@gmail.com", role = c("aut", "cre")),
person("Clayton", "Yochum", role = "aut"),
person("Caleb", "Scheidel", role = "ctb"),
Expand Down
30 changes: 24 additions & 6 deletions R/drop_file_ops.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ drop_delete <-
if (verbose) {
res
} else {
message(sprintf('%s was successfully deleted', res$metadata$path_display))
# message(sprintf('%s was successfully deleted', res$metadata$path_display))
Copy link
Contributor

Choose a reason for hiding this comment

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

why keep the old message around?

invisible(res)
}
} else {
Expand Down Expand Up @@ -281,14 +281,32 @@ drop_exists <- function(path = NULL, dtoken = get_dropbox_token()) {
if (!grepl('^/', path))
path <- paste0("/", path)
dir_name <- suppressMessages(dirname(path))
dir_listing <- drop_dir(path = dir_name, dtoken = dtoken)

if (path %in% dir_listing$path_display) {
TRUE
} else {
# In issue #142, this part below (the drop_dir call) fails when drop_dir is
# looking to see if a second level folder exists (when it doesn't.) One safe
# option is to only run drop_dir('/', recursive = TRUE) and then grep through
# that. Downside: It would take forever if this was a really large account.
#
# Other solution is to use purrr::safely to trap the error and return FALSE
# (TODO): Explore uninteded consequence of this.
safe_dir_check <-
purrr::safely(drop_get_metadata, otherwise = FALSE, quiet = TRUE)
dir_listing <- safe_dir_check(path = path, dtoken = dtoken)
# browser()
if (length(dir_listing$result) == 1) {
# This means that object does not exist on Dropbox
FALSE
} else {
# Root of path (dir_name), exists/
paths_only <- dir_listing$result$path_display

if (path %in% paths_only) {
TRUE
} else {
FALSE
}
}


}

#' Checks if an object is a file on Dropbox
Expand Down
3 changes: 2 additions & 1 deletion tests/testthat/test-02_rdrop2-upload.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,9 @@ test_that("Test that basic file ops work correctly", {
server_row_count <- nrow(y)
# Make sure the downloaded csv has the same number of rows
expect_equal(row_count, server_row_count)
expect_message(drop_delete(file_name), "successfully deleted")
# expect_message(drop_delete(file_name), "successfully deleted")
Copy link
Contributor

Choose a reason for hiding this comment

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

should probably just drop this

unlink(file_name)
drop_delete(file_name)
})

# Test upload of an image
Expand Down