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

WIP: refactor to use dplyr #36

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fmichonneau
Copy link
Member

I'm following up on @adamdsmith's suggestion of using dplyr instead of plyr to build the data frame.

This works and passes the tests for the record search. However, it fails (and crashes R) for media searches.

This is in part caused by the wonky format of the current result (data frame that contains lists) for media searches. From a quick glance, it seems that maybe the field names have changed, and we need new code to ensure that fmt_search_txt_to_df returns a flat data frame.

@mjcollin
Copy link
Contributor

I made a change to deal with the new "indexData" field that got added to API responses around September. I think this is the origin of @adamsmith's original issue. However, I went to test this PR and I now get:

Error in [[<-.data.frame(tmp, i, value = list(country = c("united states", : replacement has 5000 rows, data has 0

when running either of the idig_search_* functions. Could you test/retest?

@fmichonneau
Copy link
Member Author

Which request are you trying?

If I do:

idig_search_records(rq = list(genus = "Holothuria"))

I don't see the warning

but

idig_search_media(rq = list(genus = "Holothuria"))

still crashes R because the dat object after the while loop isn't a 2 dimensional data frame (some of the columns contain themselves data frame).

I can easily modify fmt_search_txt_to_df to return a 2d data frame, but I'm not sure all the information this contains is needed.

@mjcollin
Copy link
Contributor

Still havent' returned to this but pushing out a new release to address the issue in PR #35 since others are probably having that.

Still holding open...

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