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

More informative FileNotFoundError #2694

Merged
merged 4 commits into from Oct 23, 2021
Merged

Conversation

Starbuck5
Copy link
Contributor

@Starbuck5 Starbuck5 commented Aug 25, 2021

This PR adds a more informative FileNotFoundError to pygame's RWops system.

Take this, for example:

pygame.mixer.Sound("data/nonexistent.ogg")

Normally, if there's a problem, pygame will say:

FileNotFoundError: No such file or directory.

Which is accurate, but not super helpful, especially for newbies. And I see newbies struggle with the concept of working directories a lot.

This PR uses the os module to produce this error message instead:

FileNotFoundError: No file 'data/nonexistent.ogg' found in working directory 'C:\Users\Starbuck\Desktop'.

This could obviously still be misinterpreted, but the term "working directory" will hopefully point people in a good direction with Google searches.

And this information is omitted for absolute paths, because for those it doesn't matter what your working directory is. And if the os module is unavailable for some reason, it will just fall back to the old error message.

However, there's a problem. Most of pygame's loading functions make really complex use of the pg_RWops API, rather than using the more high level functions. So, right now, this FileNotFound will only be emitted by Sound objects failing to load. I will broaden this a lot in a companion PR, kept separate for ease of review.

Copy link
Contributor

@ankith26 ankith26 left a comment

Choose a reason for hiding this comment

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

Yes, I like the idea behind this PR. It should be really helpful for many newbies when they get FileNotFound errors in their code, and this info will help them debug better

src_c/rwobject.c Outdated Show resolved Hide resolved
@Starbuck5
Copy link
Contributor Author

Starbuck5 commented Sep 19, 2021

I also updated this so it absolute paths will do the normal python thing and say what file isn't found. I can't believe it didn't do that before!

pygame.mixer.Sound("C:/Users/Starbuck/desktop/not_a_sound.ogg") Now emits

FileNotFoundError: No such file or directory: 'C:/Users/Starbuck/desktop/not_a_sound.ogg'.

instead of just

FileNotFoundError: No such file or directory.

Also, just to be clear, I'm just doing these changes on Python 3, because I don't want to expend the effort testing them on Python 2, which is getting dropped soonTM anyway. (It's in an existing #ifdef)

Copy link
Member

@illume illume left a comment

Choose a reason for hiding this comment

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

Really great usability improvement. Thanks! 🎉

LGTM

@robertpfeiffer
Copy link
Contributor

So... this could probably be slightly inaccurate on some platforms where SDL_RWOps loads assets differently, but at least on Android (the only relevant one because PyGame officially aims to support it) the APK asset loading via JNI is done in addition to regular fopen() in the current directory, so it works out, and you can still put your assets into your P4A code directory.

Just something to keep in mind in the future...

@Starbuck5
Copy link
Contributor Author

Thank you for the reviews everyone. I think this can be delayed merging, until #2695 is merged, and that should be delayed merging until 2.0.2 is out.

@robertpfeiffer
Copy link
Contributor

Thank you for the reviews everyone. I think this can be delayed merging, until #2695 is merged, and that should be delayed merging until 2.0.2 is out.

I an surprised. I would think this is such a minor patch for such a useful feature, and it saves everybody so many grey hairs... I would want this in sooner rather than later

@Starbuck5
Copy link
Contributor Author

I an surprised. I would think this is such a minor patch for such a useful feature, and it saves everybody so many grey hairs... I would want this in sooner rather than later

It's because right now it only works on mixer.Sound, and the patch that makes it work on significantly more things, #2695, is not a minor change.

@Starbuck5 Starbuck5 added this to the 2.0.3 milestone Oct 8, 2021
@ghost
Copy link

ghost commented Oct 20, 2021

Also a feature telling to use os.path.join with images would be useful

@illume
Copy link
Member

illume commented Oct 23, 2021

Also a feature telling to use os.path.join with images would be useful

Yeah, some sort of feature for case-insensitivty/path handling would be good. Probably most code should use pathlib now rather than os.path.join though. I think this can probably be done best at the linter/code editor level though... because I'm not sure how to do it inside pygame.

@illume illume merged commit 96b2e06 into pygame:main Oct 23, 2021
@Starbuck5 Starbuck5 deleted the filenotfound branch October 24, 2021 19:11
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.

None yet

4 participants