Skip to content

Conversation

@dvirlar2
Copy link
Contributor

Changed the usage of an attributes data frame to a listed attributeList within the function, and edited the documentation to reflect those changes.

From what I've seen, most PIs input the raster layer in the web editor, so converting the single attributeList to a data frame feels like an odd side-step. If a raster is missing an attributeList, I think the better way forward would be to create an attList either through the webform or Shiny. If done through Shiny, then running set_attributes() and adding it back to the otherEntity before converting to a spatialRaster, rather than reverting an attList to a data frame.

Changed the usage of an attributes data frame to a listed attributeList within the function, and edited the documentation to reflect those changes.
Added a warning message if the coord_name argument is NULL. Warning message alerts users that EML won't validate, and provides correct string for commonly used WGS84 and NAD83 datums. Also directs users to the get_coord_list() for more information.
@dvirlar2
Copy link
Contributor Author

raster helper function edits: coord_name argument

Added a warning message if the coord_name argument is NULL. Warning message alerts users that EML won't validate, and provides correct string for commonly used WGS84 and NAD83 datums. Also directs users to the get_coord_list() for more information.

@dvirlar2 dvirlar2 marked this pull request as draft October 20, 2022 00:30
@dvirlar2 dvirlar2 marked this pull request as ready for review October 20, 2022 00:45
@dvirlar2
Copy link
Contributor Author

No longer a draft 👍🏽

Copy link
Collaborator

@jeanetteclark jeanetteclark left a comment

Choose a reason for hiding this comment

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

this looks pretty good - are all of the tests for the package passing?


if (is.null(coord_name)){
coord_name <- raster::crs(raster_obj)@projargs
#coord_name <- raster::crs(raster_obj)@projargs
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd remove this commented out line entirely

@dvirlar2
Copy link
Contributor Author

I haven't ran any formal tests within the package's test folder, but I was running tests between each push to make sure things were working and error-ing in the right/expected places. I can add a test to the end of the test_eml.R file, I'll just need a quick overview of how that works. I'll read through the testing chapter in the R Packages manual later today

@jeanetteclark
Copy link
Collaborator

Yeah formalizing what you did ad-hoc as a test, in addition to running the rest of the tests (via devtools::test()) would be good. I should have written a test when I added this function initally

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