Skip to content

Prep v2 - updated structure and choropleth map#51

Open
evanoffPNNL wants to merge 42 commits intov2from
prep-v2
Open

Prep v2 - updated structure and choropleth map#51
evanoffPNNL wants to merge 42 commits intov2from
prep-v2

Conversation

@evanoffPNNL
Copy link
Copy Markdown

This pull request sort of covers both the prep-v2 and the choropleth map issues. The structure should be mostly cleaned up and the choropleth map has been fortified for testing. Fixes #48 and fixes #49

@evanoffPNNL evanoffPNNL requested a review from crvernon April 30, 2020 15:45
Copy link
Copy Markdown
Member

@crvernon crvernon left a comment

Choose a reason for hiding this comment

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

Great work! See my comments inline.

Comment thread tests/testthat/test_create_map.R Outdated
Comment thread tests/testthat/test_create_map.R Outdated
Comment thread R/output.R Outdated
Comment thread R/output.R
Comment thread R/error_checking.R
Comment thread R/error_checking.R Outdated
#' @param map_legend_title (Character) - Text for the legend header
#' @param map_x_label (Character) - Label for x axis (default Lon)
#' @param map_y_label (Character) - Label for y axis (default Lat)
verify_map_params <- function(bin_method, bins, dpi, expand_xy, map_xy_min_max , map_title, map_palette, map_palette_reverse,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No export for this one? Just checking, may be fine

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I wasn't sure how things are done here, if exports are selectively picked or just mostly exported unless there's a particular reason for it. I see the benefits of both so let me know and I can export most things. In this case, the only thinking behind no export was simply I that I couldn't think of a reason why someone would want to call this function externally.

Comment thread R/error_checking.R
Comment thread R/error_checking.R Outdated
Comment thread R/error_checking.R
Comment thread R/error_checking.R Outdated
#' @export
return_error <- function(error, location)
{
print(paste0("There was an error at - ", location, " - building your map:"))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

should you both print and return the error? Will this be duplicative?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It would be duplicative, but that was the idea. It would certainly be fine to just print the error string and return something empty like return() also. Or did you have an alternative idea?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I believe a single error is fine.

@evanoffPNNL evanoffPNNL requested a review from crvernon May 14, 2020 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants