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

Make room_history handle a list of room_ids #9

Merged
merged 2 commits into from
Dec 16, 2021
Merged

Make room_history handle a list of room_ids #9

merged 2 commits into from
Dec 16, 2021

Conversation

GregSutcliffe
Copy link
Owner

In true R style, R history should be able to handle a vector of rooms.

This PR moves the implementation for a single room to an internal method, and then rewrites room_history to iterate over the ID list and nest the results. Here's an example:

room_ids <- c("!nmvUwBzGOuTedhIPEV:libera.chat","!pMZboYFCScZJfXmOtH:ansible.im")
since <- "2021-12-07"
room_history(room_ids, since)
# A tibble: 2 × 2
# Groups:   room [2]
  room                            events            
  <chr>                           <list>            
1 !nmvUwBzGOuTedhIPEV:libera.chat <tibble [42 × 6]> 
2 !pMZboYFCScZJfXmOtH:ansible.im  <tibble [255 × 6]>

This can then be expanded, eg by tidyr::unnest(events)

I've also included a small change to add id to the schema, as that's useful when storing and then re-running the script - events should not change, but at least we can check :)

@johrpan
Copy link
Collaborator

johrpan commented Dec 15, 2021

I added some minor style fixes that you wouldn't disagree with (I think). Before merging, I would merge those with the actual commit adding the changes.

@johrpan
Copy link
Collaborator

johrpan commented Dec 15, 2021

Otherwise, it looks good to me and seems to work as expected. I would suggest, to not have the nested tibble, but just include the room column to the result. This also facilitates the case, where you only fetch one room and unnesting is especially redundant.

@johrpan johrpan force-pushed the updates branch 2 times, most recently from e37dc9d to 16c1072 Compare December 15, 2021 12:19
@johrpan
Copy link
Collaborator

johrpan commented Dec 15, 2021

Also, one more thing: In my opinion, the internal function should be named differently (i.e. not with the prepended dot). For example room_history_internal() or single_room_history(). But I leave that to your preferences.

@GregSutcliffe
Copy link
Owner Author

GregSutcliffe commented Dec 16, 2021

Very happy with those style fixes (seriously, how did that glue get left behind?) - much thanks for that. I'll squash on merge.

I think you're right on the naming - I got the idea from meetupr but looking at other tidyverse packages I don't see it so much in use. I'll think of a name that works.

On the nesting - I think I'll merge as-is for now, as I don't have a lot of time today. I agree nesting for a single room is un-needed, but we can address that in a new PR - I'm thinking an optional parameter to the function, and the default is "nest if rooms >1, unnest if ==1)" ...

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