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

Updated player mappings. #249

Merged
merged 8 commits into from
Aug 27, 2024
Merged

Updated player mappings. #249

merged 8 commits into from
Aug 27, 2024

Conversation

TheMathNinja
Copy link
Contributor

Ok I followed all the steps! Let me know if this works. Not sure if this file really needs the blank line at the end still. Not sure how to add that; hopefully it works without a blank line.

TheMathNinja and others added 2 commits August 22, 2024 15:22
because of stupid attribute checks in new arrow version
@mrcaseb
Copy link
Member

mrcaseb commented Aug 23, 2024

My commit doesn't fix the problem because it applies only for multiple files. We need to fix the attributes in parquet files in the releases

@mrcaseb
Copy link
Member

mrcaseb commented Aug 25, 2024

Parquet problems in tests are fixed. This PR is up to you now, @tanho63

Michael Danna,Mike Danna
Ogbonnia Okoronkwo,Ogbo Okoronkwo
Henry To'oTo'o,Henry TooToo
Copy link
Member

Choose a reason for hiding this comment

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

I don’t think there is a scenario where clean names will keep these apostrophes, it is hardcoded to remove them. That probably makes the entry redundant.

Copy link
Member

@tanho63 tanho63 left a comment

Choose a reason for hiding this comment

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

Remainder of this looks good. Can you add a line to NEWS.md describing the changes you made?

@TheMathNinja
Copy link
Contributor Author

Yes but also a quick question. This was an immediate fix I submitted for an auction that’s now over. Now that I have proof of concept in submitting the PR would it be better to submit just this now or submit a more full one with a complete mapping fix? Or should I do a second one for the complete fix?

@tanho63
Copy link
Member

tanho63 commented Aug 25, 2024

submit now and then PR the remainder after if you want

@TheMathNinja
Copy link
Contributor Author

Also, I now have the full list of names I actually want to change. Should I wait for this PR to go through first or just close this one and re-open a new one with the full list?

@tanho63 tanho63 enabled auto-merge (squash) August 27, 2024 16:42
@tanho63 tanho63 merged commit a856def into nflverse:main Aug 27, 2024
6 of 7 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