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

Target arbitrary and open rectangles; fixes #8, fixes #313 #314

Merged
merged 18 commits into from Apr 4, 2017

Conversation

Projects
None yet
3 participants
@jennybc
Member

jennybc commented Apr 3, 2017

No description provided.

jennybc added some commits Mar 31, 2017

Delete xls[x]_col_[names|types], FINALLY
I imagine these were useful at an earlier stage of development. But I never use them  and I'm tired of keeping them current.

@jennybc jennybc requested a review from hadley Apr 3, 2017

@jennybc jennybc changed the title from [WIP] Target arbitrary and open rectangles; closes #8 to [WIP] Target arbitrary and open rectangles; fixes #8 Apr 3, 2017

#'
#' @examples
#' \dontrun{
#' gs_gap() %>% gs_read(ws = 2, range = "A1:D8")

This comment has been minimized.

@hadley

hadley Apr 3, 2017

Member

Just a note that you need to update this example

This comment has been minimized.

@jennybc

jennybc Apr 3, 2017

Member

Fixed in 94b96c5

n_max <- check_non_negative_integer(n_max, "n_max")
n_read <- if (has_col_names) n_max + 1 else n_max
## min_row = -2 is a special flag for 'read no rows'
limits <- c(min_row = if (n_read > 0) skip else -2,

This comment has been minimized.

@hadley

hadley Apr 3, 2017

Member

tidyverse style is

limits <- c(
  min_row = ...,
  max_row = ...,
  ...
)

This comment has been minimized.

@jennybc
#include "XlsCell.h"
class CellLimits {
std::map<std::string,int> limits_;

This comment has been minimized.

@hadley

hadley Apr 3, 2017

Member

Stylewise, it's a bit odd to use a map here. Why not just make minRow, minCol, etc fields?

This comment has been minimized.

@jennybc

jennybc Apr 4, 2017

Member

Using fields now 80125a9

@@ -156,6 +169,26 @@ standardise_sheet <- function(sheet, sheet_names) {
}
}
standardise_limits <- function(range, skip, n_max, has_col_names) {

This comment has been minimized.

@hadley

hadley Apr 3, 2017

Member

I'd recommend a brief comment indicating that this returns 0-indexed data structure, -1 = all, -2 = none.

This comment has been minimized.

@jennybc
contains(this->min_col(), this->max_col(), cell.col());
}
void print() {

This comment has been minimized.

@hadley

hadley Apr 3, 2017

Member

@jimhester what's the right way to make Rcout << CellLimits() work?

This comment has been minimized.

@jimhester

This comment has been minimized.

@jennybc

jennybc Apr 4, 2017

Member

Good to know! I will probably leave this as is, because it was solely for development/debugging and may very well get deleted.

}
// fix this!
// maybe I should use a map?
IntegerVector limits = as<IntegerVector>(input_limits);

This comment has been minimized.

@hadley

hadley Apr 3, 2017

Member

Why are you doing coercion here instead of in the function defintion?

I agree that a std::map<std::string, int> would be better (and I'm pretty sure Rcpp does the conversion)

This comment has been minimized.

@jennybc

jennybc Apr 4, 2017

Member

Why are you doing coercion here instead of in the function defintion?

Just not knowing any better. Fixed 81b7625.

if (ncol_ < it->col()) {
ncol_ = it->col();
if (shim) {

This comment has been minimized.

@hadley

hadley Apr 3, 2017

Member

I think you need a brief comment here outlining the strategy

This comment has been minimized.

@jennybc

jennybc Apr 4, 2017

Member

Things have gotten simpler (at least, when viewed up close) and better commented 6ce4b4e.

jennybc added some commits Apr 3, 2017

Streamline and comment geometry work
  * Kill firstRow_ and secondRow_
  * Kill empirical_
  * Don't even load cells outside nominal_.
  * Eliminate gratuitous trips through cells_ related to learning or setting geometry.

jennybc added some commits Apr 4, 2017

@hadley

hadley approved these changes Apr 4, 2017

@jennybc jennybc changed the title from [WIP] Target arbitrary and open rectangles; fixes #8 to Target arbitrary and open rectangles; fixes #8 Apr 4, 2017

@jennybc jennybc changed the title from Target arbitrary and open rectangles; fixes #8 to Target arbitrary and open rectangles; fixes #8, fixes #313 Apr 4, 2017

@jennybc jennybc merged commit b80ac11 into tidyverse:master Apr 4, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@jennybc jennybc deleted the jennybc:target-cell-range branch Apr 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment