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.

Sign up for GitHub

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

Jump to bottom

Add player attributes from fbref [Closes #47] #413

Closed
wants to merge 21 commits into from
Closed

Add player attributes from fbref [Closes #47] #413

wants to merge 21 commits into from

Conversation

aymennasri
Copy link
Contributor

@aymennasri aymennasri commented Jan 29, 2025 •
edited
Loading

[#47] All good except for the age column that I couldn't extract due to it being inside of a <nobr> tag.

@aymennasri aymennasri force-pushed the add_player_attributes_fbref branch 2 times, most recently from dc3be8e to 6bfd87c Compare January 29, 2025 11:06
@aymennasri
Copy link
Contributor Author

Fixed the age column issue by manually calculating it, should be robust enough to be merged directly.

@tonyelhabr tonyelhabr mentioned this pull request Jan 31, 2025
Closed
@aymennasri aymennasri force-pushed the add_player_attributes_fbref branch from 7ec02d0 to fdbc25b Compare January 31, 2025 15:42
@JaseZiv JaseZiv mentioned this pull request Feb 1, 2025
Merged
@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

@tonyelhabr tonyelhabr self-requested a review February 9, 2025 02:50
@tonyelhabr
Copy link
Collaborator

evidently i've messed up the git history here. i'll fix when i get a chance

@aymennasri
Copy link
Contributor Author

@tonyelhabr it should've been ordered from #414 to #413 to #412

@tonyelhabr
Copy link
Collaborator

@tonyelhabr it should've been ordered from #414 to #413 to #412

I'll fix tomorrow, just haven't had time this weekend. It's solve-able by incrementing versions for 413 and 414

@tonyelhabr tonyelhabr mentioned this pull request Feb 10, 2025
Merged
@tonyelhabr
Copy link
Collaborator

changes submitted in #415

@tonyelhabr tonyelhabr closed this Feb 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tonyelhabr tonyelhabr Awaiting requested review from tonyelhabr

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aymennasri @JaseZiv @tonyelhabr