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

Fix fb_season_team_stats() #390

Merged
merged 4 commits into from
Jul 20, 2024
Merged

Fix fb_season_team_stats() #390

merged 4 commits into from
Jul 20, 2024

Conversation

tonyelhabr
Copy link
Collaborator

Fixes #389

@tonyelhabr tonyelhabr requested a review from JaseZiv July 20, 2024 11:37

# have included this to differentiate between how different leagues handle ladders/tables
league_tables <- season_stats_page %>% rvest::html_nodes("#content .table_wrapper") %>% rvest::html_elements("h2") %>% rvest::html_text()

# we either want to detect the presence of League Table or Regular season and get the index of that to be able to extractg the table we want
if(length(grep("league table", tolower(league_tables))) > 0) {
league_tables_idx <- grep("league table", tolower(league_tables))
if(length(grep(competition_name, league_tables)) > 0) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

MLS is an ongoing league. We can see that "Regular Season" is still used, so we still need this if-else clause.

But for every other league that I checked, the competition name is now used for completed seasons.

Copy link
Owner

@JaseZiv JaseZiv left a comment

Choose a reason for hiding this comment

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

LGTM.

Thanks for addressing this one so quickly @tonyelhabr!

sd_meta.R Outdated
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

looks like an unintentionally committed file

@tonyelhabr
Copy link
Collaborator Author

@JaseZiv ah hold up, got one more code change i need to make 🤦

@tonyelhabr
Copy link
Collaborator Author

@JaseZiv ah hold up, got one more code change i need to make 🤦

ok we're good now. needed to rename season_url to single_season_url in two places

@JaseZiv
Copy link
Owner

JaseZiv commented Jul 20, 2024

@JaseZiv ah hold up, got one more code change i need to make 🤦

ok we're good now. needed to rename season_url to single_season_url in two places

Awesome! Happy for me to wait until the checks have run before merging @tonyelhabr?

@tonyelhabr
Copy link
Collaborator Author

@JaseZiv ah hold up, got one more code change i need to make 🤦

ok we're good now. needed to rename season_url to single_season_url in two places

Awesome! Happy for me to wait until the checks have run before merging @tonyelhabr?

only failing tests are for unrelated functions (transfermarket stuff, fb_league_stats() due to RFC error). going to go ahead and merge to unblock @Paulj1989

@tonyelhabr tonyelhabr merged commit b9a9181 into main Jul 20, 2024
0 of 2 checks passed
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.

fb_season_team_stats() returning error when stat_type = "league_table"
2 participants