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

Add documentation and tests to check for exoticCategory column #136

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

RichardLitt
Copy link
Contributor

Description

I've added documentation strings to all of the methods which can return exoticCategory. I've also added a test to one of these, showing how I think we should test for it - but I would like advice on whether or not this is the right approach before copying this to other tests.

Related Issue

Closes #129.

@sebpardo
Copy link
Contributor

Hi @RichardLitt, apologies for taking so long to chime in. The documentation is great and the tests make sense. In the past we've kept track of number of columns as a way way to test this but this can break the tests when the API changes, which has happened every so often. I would suggest adding the following line in the test file to keep so that we know that the exoticCategory column is additional to the other ones: expect_equal(ncol(out1), ncol(out2) + 1)

@RichardLitt
Copy link
Contributor Author

Thank you! Done.

@sebpardo
Copy link
Contributor

Looks like I was wrong in assuming columns would be the same between checklists with and without exotics:

── Failure ('test-ebirdchecklist.R:27:5'): ebirdchecklist succeeds reproducibly ──
ncol(out1) not equal to ncol(out2) + 1.
1/1 mismatches
[1] 21 - 28 == -7

This seems to be, in part, because the checklist used in the example without exotics (out2, https://ebird.org/checklist/S89475689) contains media:

setdiff(names(out1), names(out2)) # columns in out1 but not it out2
#> [1] "durationHrs"    "subComments"    "exoticCategory"
#> [4] "auxCode"       
setdiff(names(out2), names(out1))  # columns in out2 but not it out1
#>  [1] "projId"                     
#>  [2] "groupId"                    
#>  [3] "submissionMethodCode"       
#>  [4] "submissionMethodVersion"    
#>  [5] "submissionMethodVersionDisp"
#>  [6] "fieldName"                  
#>  [7] "howManyAtleast"             
#>  [8] "howManyAtmost"              
#>  [9] "present"                    
#> [10] "audioCounts"      

While it might be ideal to mention in the documentation that certain columns might or might not be returned depending of whether the checklist contains media and/or exotic species, I think for now we should just leave so we can get this version uploaded to CRAN, which means removing the ncol comparison I suggested in the documentation. I'm very sorry for leading you astray.

There might be other function tests where comparing column numbers might be appropriate, I'll leave it up to you to decide whether to include or not.

@RichardLitt
Copy link
Contributor Author

I've just removed it for now. No worries! Thanks for checking in on this.

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.

ebirdregion() output sometimes includes exoticCategory field
2 participants