8000 Pathlib support for most pygame functions by ankith26 · Pull Request #2366 · pygame/pygame · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Pathlib support for most pygame functions #2366

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 8000 to your account

Jump to bottom
Merged
merged 2 commits into from
Dec 2, 2020
Merged

Pathlib support for most pygame functions #2366

merged 2 commits into from
Dec 2, 2020

Conversation

ankith26
Copy link
Contributor
@ankith26 ankith26 commented Nov 28, 2020 •
edited
Loading

This PR adds Pathlib support to most pygame functions where a file is taken in as an argument. This includes pygame.image, pygame.mixer, pygame.music, pygame.font, pygame.ftfont and pygame.freetype (basically, wherever the internal function pg_EncodeString is called)
It was a relatively simple addition, yet much needed as many people have asked for this.
There was a previous attempt to do this, but that PR did not work on py 3.5. This PR makes sure it does.

Issues this PR aims to close are #1129 #1635

Unit tests and docs coming soon :)

@ankith26
Copy link
Contributor Author

Again....
Forgot [skip ci]

@ankith26
Copy link
Contributor Author

I did a bit of local testing. And the functions accept pathlib objects.
I just updated the unittests, and let the CI run, because I wanted to see whether it’s fine on all platforms and versions.

@ankith26
Copy link
Contributor Author
ankith26 commented Nov 30, 2020 •
edited
Loading

Looks like appveyor is not in a mood to work today.
Travis arm64 is not happy too, but that’s not my PR

EDIT: pygame.org appears to be down ATM, that explains appveyor failures

EDIT: also Manylinux and Mac builds are happy. So only code-reviews and docs are left

illume
illume approved these changes Dec 2, 2020
View reviewed changes
Copy link
Member
@illume illume left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Cool. A very nice improvement. Thanks 👍

I reran the appveyor jobs after fixing the website and they passed :)

I haven't reviewed the C code in great detail, but it looks good at first glance.

Could probably do with tests for loading images, and sounds. But they could come later IMHO. Leaving out python 2 support is fine IMHO... if someone else wants to add support for pathlib/pathlib2 backport they can later. Docs and types could also come later. We can leave the issue open with these points.

If you're happy with it, we can merge this in now.

@ankith26
Copy link
Contributor Author
ankith26 commented Dec 2, 2020

Yup. We can merge it anytime.
There is no point of python2 compatibility in this at all. Pathlib itself is a module introduced in python 3.4
As for the tests, I have included a basic test that should cover for all. But for extra safety, maybe it might be good to include a few tests on a per-function basis as well.

And yeah. I would like to to docs/types. If you would like to merge this, I can open a separate PR. Whichever is convenient for you :)

@illume illume merged commit f4b01f0 into main Dec 2, 2020
@illume illume deleted the ankith26-pathlib-support branch December 2, 2020 19:16
@illume illume mentioned this pull request Dec 13, 2020
Closed
4 tasks
@illume illume added this to the 2.0.1 milestone Aug 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@illume illume illume approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@ankith26 @illume
0