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

added fields to find_groups() to retrieve optional fields from M… #48

Merged
merged 14 commits into from
Oct 17, 2020

Conversation

benubah
Copy link
Collaborator

@benubah benubah commented May 31, 2019

I added fields to find_groups() to retrieve optional fields from Meetup API

The find/groups method (https://www.meetup.com/meetup_api/docs/find/groups/ ) within the Meetup API has several optional fields that are not returned by default.
They will be returned when you pass them to the fields attribute as part of your API query. This functionality is now provided within meetupr. In meetupr, after the query, these fields can then be retrieved from the resource tibble attribute already defined in find_groups().

benubah added 3 commits May 31, 2019 09:41
…etup API

find/groups method within the Meetup API has several optional fields that are not returned by default.
They will be returned when you pass them to the `fields` attribute as part of your API query. This functionality is now provided within meetupr. In meetupr, after the query, these fields can then be retrieved from the `resource` tibble attribute already defined in find_groups().
@benubah
Copy link
Collaborator Author

benubah commented May 31, 2019

fields has also been added to get_events() for the same purpose of returning optional fields that are not returned by default

@ledell ledell requested review from ledell and LucyMcGowan June 3, 2019 18:49
@ledell
Copy link
Member

ledell commented Jun 6, 2019

@benubah This looks like a nice usability feature. I think if we are going to add something like this, it would make sense to add it to all our functions... what do you think, @LucyMcGowan?

It also may make sense to call it something like extra_fields or additional_fields to emphasize that these are columns that are in addition to the default ones? I am open to ideas.

@ledell ledell self-assigned this Jun 6, 2019
@LucyMcGowan
Copy link
Member

This looks good! I am torn on whether it should be called extra_fields or additional_fields versus fields - the way you currently have it matches the meetup API docs since they call it fields, but the way @ledell proposes is a bit clearer (to me) - I was initially confused and thought you'd need to list all desired fields in that parameter, not just the additional ones

@ledell
Copy link
Member

ledell commented Jun 6, 2019

@benubah @LucyMcGowan I am noticing now that this is an actual parameter in the API (I thought it was just made up parameter for extra usability), so I understand now why it was named fields (to match API) even though it's somewhat of a misnomer. @benubah Are there any other API methods that we already implement that have a fields parameter other than get_events() and find_groups()? If so, we can update them all at once.

It's a bit confusing because there's no way for the user to know in advance what extra fields they want unless they carefully read the API docs... would it make sense to hard-code the extra field options so the user is not digging into API docs to use the parameter effectively? e.g. fields = c("event_hosts", "featured", ...)

Trying out an example (without using fields), this is what I see:

urlname <- "rladies-nashville"
past_meetings <- get_events(urlname = urlname, event_status = "past", api_key = api_key)

Here's what we return in our tibble:

> names(past_meetings)
 [1] "id"              "name"            "created"         "status"          "time"            "local_date"      "local_time"     
 [8] "waitlist_count"  "yes_rsvp_count"  "venue_id"        "venue_name"      "venue_lat"       "venue_lon"       "venue_address_1"
[15] "venue_city"      "venue_state"     "venue_zip"       "venue_country"   "description"     "link"            "resource" 

And here are the remaining fields available by default in the resource list (raw data):

> names(past_meetings$resource[[1]])
 [1] "created"                "duration"               "id"                     "name"                   "date_in_series_pattern"
 [6] "status"                 "time"                   "local_date"             "local_time"             "updated"               
[11] "utc_offset"             "waitlist_count"         "yes_rsvp_count"         "venue"                  "group"                 
[16] "link"                   "description"            "how_to_find_us"         "visibility"             "pro_is_email_shared"  

@LucyMcGowan
Copy link
Member

One option could be to include a link to the API docs directly with a list of potential fields (if such a thing exists) in the docs about this new parameter

@ledell
Copy link
Member

ledell commented Jun 6, 2019

@benubah Here's what I suggest -- let's leave fields defaulting to NULL, but in the fields parameter description, let's hardcode the list of options so the user does not have to leave the R terminal and go to the web to look them up.

R/get_events.R Outdated
@@ -9,7 +9,7 @@
#' * proposed
#' * suggested
#' * upcoming
#'
#' @param fields Character or characters separated by comma (e.g "event_hosts" or "event_hosts, past_event_count_inclusive").
Copy link
Member

Choose a reason for hiding this comment

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

I tried passing "event_hosts, past_event_count_inclusive" and it did not work... I don't see "past_event_count_inclusive" on the list of additional fields: https://www.meetup.com/meetup_api/docs/2/events/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for reviewing this. The additional fields are stored in resource - last attribute in the tibble, so the following works for me:

urlname <- "rladies-ames"
#get past meetings of a single group
past_meetings <- get_events(urlname = urlname, event_status = "past", fields = "event_hosts, past_event_count_inclusive", api_key = api_key)
# get events hosts of single past meeting
single_event <- past_meetings$resource[[1]]$event_hosts

These additional fields are not available in v2 of the meetup API. You can see them in v3 here: https://www.meetup.com/meetup_api/docs/:urlname/events/#list

Copy link
Member

@ledell ledell Jun 6, 2019

Choose a reason for hiding this comment

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

The past_event_count_inclusive field is not returned:

> names(past_meetings$resource[[1]])
 [1] "created"                "duration"               "id"
 [4] "name"                   "date_in_series_pattern" "status"
 [7] "time"                   "local_date"             "local_time"
[10] "rsvp_close_offset"      "updated"                "utc_offset"
[13] "waitlist_count"         "yes_rsvp_count"         "venue"
[16] "group"                  "link"                   "description"
[19] "how_to_find_us"         "visibility"             "pro_is_email_shared"
[22] "event_hosts"
> past_meetings$resource[[1]]$past_event_count_inclusive
NULL

Copy link
Member

@ledell ledell Jun 6, 2019

Choose a reason for hiding this comment

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

Are you able to see it in your results? I see that it's listed in v3 docs but not v2 so I wonder if that's causing some issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, I don't see past_event_count_inclusive. This looks like a problem from within the API itself, because, I tried group_past_event_count and it returned a value under group list in resource.

past_meetings <- get_events(urlname = urlname, event_status = "past", fields = "event_hosts, group_past_event_count", api_key = api_key)

names(past_meetings$resource[[1]]$group)

So I guess, it would be better to take out past_event_count_inclusive for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Where the optional fields parameters are in the range of 30 - 40 different parameters for get_events(), would you like to hardcode say, all 40 parameters in the fields description? I see that the parameters are many in v3.

@ledell
Copy link
Member

ledell commented Jun 6, 2019

Another thing to decide on -- right now, if you want to get multiple fields, you pass a single comma delimited string. In R, people would be used to passing a character vector with multiple string values... so which input type is better?

Current:

past_meetups <- get_events(urlname = urlname, 
                           event_status = "past", 
                           fields = "event_hosts, featured", 
                           api_key = api_key)

Other option:

past_meetups <- get_events(urlname = urlname, 
                           event_status = "past", 
                           fields = c("event_hosts", "featured"), 
                           api_key = api_key)

Or we can support both.

@benubah
Copy link
Collaborator Author

benubah commented Jun 6, 2019

@ledell @LucyMcGowan Thank you for looking into this. I will look into all the concerns raised and will feedback soon.

@benubah
Copy link
Collaborator Author

benubah commented Jun 7, 2019

Another thing to decide on -- right now, if you want to get multiple fields, you pass a single comma delimited string. In R, people would be used to passing a character vector with multiple string values... so which input type is better?

Current:

past_meetups <- get_events(urlname = urlname, 
                           event_status = "past", 
                           fields = "event_hosts, featured", 
                           api_key = api_key)

Other option:

past_meetups <- get_events(urlname = urlname, 
                           event_status = "past", 
                           fields = c("event_hosts", "featured"), 
                           api_key = api_key)

Or we can support both.

Yes, I have now added support for both.

@benubah
Copy link
Collaborator Author

benubah commented Jun 28, 2019

@benubah Here's what I suggest -- let's leave fields defaulting to NULL, but in the fields parameter description, let's hardcode the list of options so the user does not have to leave the R terminal and go to the web to look them up.

@ledell Could you please look into this:

" where the optional fields parameters are in the range of 30 - 40 different parameters for get_events() in the meetup API, would you like to hard-code say, all 40 parameters in the fields description while documenting the function? I see that the parameters are many in v3."

@ledell
Copy link
Member

ledell commented Jul 4, 2019

@benubah

" where the optional fields parameters are in the range of 30 - 40 different parameters for get_events() in the meetup API, would you like to hard-code say, all 40 parameters in the fields description while documenting the function? I see that the parameters are many in v3."

Feel free to use your judgement on this one... I think you're right that 30-40 params would be too much for the docs. Maybe if there are >10, we should just reference a URL that lists them?

@RickPack
Copy link
Collaborator

I am presenting about this work as one of the Google Summer of Code 2019 mentors on Tuesday, October 22, 2019. Might someone give me a quick comment about the current status that helps one understand why Ben's work has not yet been merged into the main meetupr repo? I see there are conflicting files.
Might you have thoughts about how I or someone else can help?

@ledell
Copy link
Member

ledell commented Oct 6, 2020

Hi @benubah and @RickPack -- sorry for dropping the ball on this. I was low on bandwidth at the time we were having this conversation, but I am revisiting some of the outstanding meetupr tasks this week.

It looks like the conflicts are related to the OAuth changes made in Aug 2019. Would one of you be able to fix the conflicts? Once these are fixed, we can get this merged into the package!

The other topic that never got fully resolved was whether it was better to name the parameter something more accurate (like extra_fields), or whether to just re-use the "true" name, fields. I am ok to keep to the "true" name -- anyone else have final thoughts on that?

@ledell ledell self-requested a review October 6, 2020 22:43
@RickPack
Copy link
Collaborator

RickPack commented Oct 8, 2020 via email

@ledell
Copy link
Member

ledell commented Oct 8, 2020

Hi @RickPack that's great! If you have time to resolve the conflicts over the weekend, then I can merge it as soon as you tell me you're done. Thanks!

@RickPack
Copy link
Collaborator

RickPack commented Oct 12, 2020 via email

@ledell
Copy link
Member

ledell commented Oct 15, 2020

I'm just tagging @benubah to make sure he sees your comments, @RickPack. Hopefully we can get the conflicts resolved and this merged soon! :-)

@ledell
Copy link
Member

ledell commented Oct 15, 2020

Also @benubah please add your name in the DESCRIPTION file as a contributor to the package (role = "ctb")! I'd love to get your three PRs merged soon.

@RickPack
Copy link
Collaborator

RickPack commented Oct 15, 2020 via email

@benubah
Copy link
Collaborator Author

benubah commented Oct 16, 2020

Hi @RickPack @ledell , Sorry I have not had time to respond.
I hope to look at this over this weekend. Let me see if I can resolve all these conflicts at once.
Thanks for inviting me to be a contributor.

@benubah
Copy link
Collaborator Author

benubah commented Oct 16, 2020

All fixed!

Copy link
Member

@ledell ledell left a comment

Choose a reason for hiding this comment

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

Thanks @benubah!

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.

4 participants