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

Add suspensions from transfermarkt [Closes #411] #412

Merged
merged 9 commits into from
Feb 9, 2025

Conversation

aymennasri
Copy link
Contributor

Added a half baked solution to #411 as I tried my best to match the authors approach in other functions.
get_risk_of_suspension() with a url is the only thing working and I will need assistance if the author deems the pull request worthy enough.

get_risk_of_suspension() with a url is the only thing working.
@aymennasri aymennasri changed the title Add suspensions Add suspensions [Satisfies #411] Jan 28, 2025
@aymennasri
Copy link
Contributor Author

aymennasri commented Jan 28, 2025

Fixed everything with the functions, should be ready to merge [Closes #411]

@aymennasri aymennasri changed the title Add suspensions [Satisfies #411] Add suspensions [Closes #411] Jan 28, 2025
@aymennasri aymennasri changed the title Add suspensions [Closes #411] Add suspensions from transfermarkt [Closes #411] Jan 29, 2025
@tonyelhabr
Copy link
Collaborator

is it possible to add a unit test?

@aymennasri
Copy link
Contributor Author

for what exactly?

@tonyelhabr
Copy link
Collaborator

for what exactly?

for the functions added in this PR. something as simple as checking rows in an example call, e.g. here

@aymennasri
Copy link
Contributor Author

aymennasri commented Feb 1, 2025

for what exactly?

for the functions added in this PR. something as simple as checking rows in an example call, e.g. here

@tonyelhabr You can't really predict the number of suspensions in a legaue and it changes constantly alongside other stats, which makes a lot of the tests in the package to be faulty. (like the injuries in a league)

For example, the Premier League has no suspended players for now but that doesn't mean that the function is faulty.

@JaseZiv
Copy link
Owner

JaseZiv commented Feb 1, 2025

for what exactly?

for the functions added in this PR. something as simple as checking rows in an example call, e.g. here

@tonyelhabr You can't really predict the number of suspensions in a legaue and it changes constantly alongside other stats, which makes a lot of the tests in the package to be faulty. (like the injuries in a league)

For example, the Premier League has no suspended players for now but that doesn't mean that the function is faulty.

@aymennasri you don't just need to test for row numbers... you can also write unit tests to ensure the columns/data points you're wanting to scrape are still being presented (even if no data for the particular player)...

@JaseZiv
Copy link
Owner

JaseZiv commented Feb 1, 2025

@aymennasri can you also increment the patch number of the package version in DESCRIPTION for all PRs based on which order you want the three PRs matched (#412, #413, #414). See here for an explainer on versioning. For the PR that will ultimately create the latest version, can you also update NEWS.md, following the same conventions we're using in there? Thanks

@aymennasri
Copy link
Contributor Author

for what exactly?

for the functions added in this PR. something as simple as checking rows in an example call, e.g. here

@tonyelhabr You can't really predict the number of suspensions in a legaue and it changes constantly alongside other stats, which makes a lot of the tests in the package to be faulty. (like the injuries in a league)
For example, the Premier League has no suspended players for now but that doesn't mean that the function is faulty.

@aymennasri you don't just need to test for row numbers... you can also write unit tests to ensure the columns/data points you're wanting to scrape are still being presented (even if no data for the particular player)...

Done.

@tonyelhabr tonyelhabr self-requested a review February 7, 2025 16:58
@tonyelhabr
Copy link
Collaborator

lgtm. tested out the code for various leagues and it seemed to work fine. i added examples and code to the vignette

@JaseZiv
Copy link
Owner

JaseZiv commented Feb 8, 2025

Thanks so much for reviewing this @tonyelhabr.

@aymennasri
Copy link
Contributor Author

@tonyelhabr @JaseZiv is there anything i should change in order to have my PRs merged?

Copy link
Collaborator

@tonyelhabr tonyelhabr left a comment

Choose a reason for hiding this comment

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

test failures are unrelated

@tonyelhabr tonyelhabr merged commit 75a463f into JaseZiv:main Feb 9, 2025
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.

3 participants