Different players with same name

jplecavalier commented 4 years ago

There is currently a problem with the util_map_player_ids() function when different players share the same name. I don't know yet what's the best solution to fix this, but we should at least make the error message more meaningful.

The non-vectorized function util_map_player_id() works well.

nhlapi:::util_map_player_id("Ryan O'Reilly")
##  [1] 8475158 8481026

The current behaviour of util_map_player_ids() is

nhlapi:::util_map_player_ids(c("Joe Sakic", "Ryan O'Reilly"))
##  Error in vapply(playerNames, util_map_player_id, FUN.VALUE = integer(1),  : 
##    values must be length 1,
##   but FUN(X[[2]]) result is length 2

I have two different proposition:

Option 1: Throw a meaningful error

nhlapi:::util_map_player_ids(c("Joe Sakic", "Ryan O'Reilly"))
##  Error in nhlapi:::util_map_player_ids(c("Joe Sakic", "Ryan O'Reilly"))  : 
##   duplicated name for Ryan O'Reilly: 8475158 8481026

Option 2: Return a list of vector with every IDs that match the name as below

nhlapi:::util_map_player_ids(c("Joe Sakic", "Ryan O'Reilly"))
##  $`Joe Sakic`
##  [1] 8451101
##  
##  $`Ryan O'Reilly`
##  [1] 8475158 8481026

However, this would break some other function using util_map_player_ids(), we should then address them accordingly.

This time, I have three different propositions:

Option 2.1: Throw an error

nhl_players(c("Joe Sakic", "Ryan O'Reilly"))
##  Error in nhl_players(c("Joe Sakic", "Ryan O'Reilly"))  : 
##   duplicated name for Ryan O'Reilly: 8475158 8481026
##  please use the 'playerIds' argument

Option 2.2: Default to the most recent (young) and throw a warning

nhl_players(c("Joe Sakic", "Ryan O'Reilly"))
##         id      fullName                   link firstName lastName primaryNumber  birthDate birthCity birthStateProvince birthCountry nationality height weight active rookie
##  1 8451101     Joe Sakic /api/v1/people/8451101       Joe    Sakic            19 1969-07-07   Burnaby                 BC          CAN         CAN 5' 11"    195  FALSE  FALSE
##  2 8481026 Ryan O'Reilly /api/v1/people/8481026      Ryan O'Reilly            44 2000-03-21 Southlake                 TX          USA         USA  6' 2"    212   TRUE   TRUE
##    shootsCatches rosterStatus primaryPosition.code primaryPosition.name primaryPosition.type primaryPosition.abbreviation                                                url
##  1             L            I                    C               Center              Forward                            C https://statsapi.web.nhl.com/api/v1/people/8451101
##  2             R            N                    R           Right Wing              Forward                           RW https://statsapi.web.nhl.com/api/v1/people/8481026
##                                                                                                                                                                             copyright
##  1 NHL and the NHL Shield are registered trademarks of the National Hockey League. NHL and NHL team marks are the property of the NHL and its teams. © NHL 2020. All Rights Reserved.
##  2 NHL and the NHL Shield are registered trademarks of the National Hockey League. NHL and NHL team marks are the property of the NHL and its teams. © NHL 2020. All Rights Reserved.
##    currentAge alternateCaptain captain currentTeam.id  currentTeam.name currentTeam.link
##  1         NA               NA      NA             NA              <NA>             <NA>
##  2         20            FALSE   FALSE             17 Detroit Red Wings /api/v1/teams/17
##  Warning message:
##  duplicated name for Ryan O'Reilly: 8475158 8481026
##  ID 8481026 was use by default
##  please use the 'playerIds' argument to select another one

Option 2.3: Return all records for the duplicate name with a warning

nhl_players(c("Joe Sakic", "Ryan O'Reilly"))
##         id      fullName                   link firstName lastName primaryNumber  birthDate birthCity birthStateProvince birthCountry nationality height weight active rookie
##  1 8451101     Joe Sakic /api/v1/people/8451101       Joe    Sakic            19 1969-07-07   Burnaby                 BC          CAN         CAN 5' 11"    195  FALSE  FALSE
##  2 8475158 Ryan O'Reilly /api/v1/people/8475158      Ryan O'Reilly            90 1991-02-07   Clinton                 ON          CAN         CAN  6' 1"    216   TRUE  FALSE
##  3 8481026 Ryan O'Reilly /api/v1/people/8481026      Ryan O'Reilly            44 2000-03-21 Southlake                 TX          USA         USA  6' 2"    212   TRUE   TRUE
##    shootsCatches rosterStatus primaryPosition.code primaryPosition.name primaryPosition.type primaryPosition.abbreviation                                                url
##  1             L            I                    C               Center              Forward                            C https://statsapi.web.nhl.com/api/v1/people/8451101
##  2             L            Y                    C               Center              Forward                            C https://statsapi.web.nhl.com/api/v1/people/8475158
##  3             R            N                    R           Right Wing              Forward                           RW https://statsapi.web.nhl.com/api/v1/people/8481026
##                                                                                                                                                                             copyright
##  1 NHL and the NHL Shield are registered trademarks of the National Hockey League. NHL and NHL team marks are the property of the NHL and its teams. © NHL 2020. All Rights Reserved.
##  2 NHL and the NHL Shield are registered trademarks of the National Hockey League. NHL and NHL team marks are the property of the NHL and its teams. © NHL 2020. All Rights Reserved.
##  3 NHL and the NHL Shield are registered trademarks of the National Hockey League. NHL and NHL team marks are the property of the NHL and its teams. © NHL 2020. All Rights Reserved.
##    currentAge alternateCaptain captain currentTeam.id  currentTeam.name currentTeam.link
##  1         NA               NA      NA             NA              <NA>             <NA>
##  2         29            FALSE   FALSE             19   St. Louis Blues /api/v1/teams/19
##  3         20            FALSE   FALSE             17 Detroit Red Wings /api/v1/teams/17
##  Warning message:
##  duplicated name for Ryan O'Reilly: 8475158 8481026
##  a different row for each of them was created
##  please use the 'playerIds' argument to only select one of them

The same logic would apply for nhl_players_seasons() and nhl_players_allseasons().

Just tell me which of the proposition you would be more comfortable with, I will take care of it and open a PR.

jozefhajnala commented 4 years ago

Many thanks for reporting, I will hopefully take a look into this this week!

jozefhajnala commented 4 years ago

Hi @jplecavalier, I merged the fix to master, could you possibly check whether your issue is fixed (e.g. by installing the dev version from master):

# With remotes
remotes::install_github("jozefhajnala/nhlapi")

# Or with devtools
devtools::install_github("jozefhajnala/nhlapi")

Thanks again for reporting and testing!

jozefhajnala / nhlapi