Skip to content

Implementing warnings system and pygame.debug #2944

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

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

gresm
Copy link
Contributor

@gresm gresm commented Jun 19, 2024

Fixes #2894

Notes (mostly to self):

  • Should pygame.base.get_warnings_filter/pygame.base.set_warnings_filter be public API (likely not)?
  • Current idea of warning priorities (can be changed):
    • 0 is the most impactful - if a warning of its type is triggered, probably the code won't work
    • 1 is less severe - like deprecation warnings
    • 2 is least important - warnings might be pedantic or about conventions or detection of common pitfalls that can contain false positives, so is suppressed by default
  • This PR does not introduce new warnings to guide users, only sets up a system for filtering them. Porting opinionated warnings pygame/pygame#1682 or implementing something like that is still needed, but in a separate PR.

@gresm gresm marked this pull request as ready for review June 20, 2024 21:31
@gresm gresm requested a review from a team as a code owner June 20, 2024 21:31
gresm added 2 commits June 21, 2024 11:43
Update outputted warning in docs to reflect changes.
@gresm gresm requested a review from bilhox June 21, 2024 10:30
Copy link
Contributor

@bilhox bilhox left a comment

Choose a reason for hiding this comment

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

Everything in the side of documentation seems fine. Maybe if you add a code example that makes full use of the function argument, it would be more helpful to users.

@damusss damusss added the New API This pull request may need extra debate as it adds a new class or function to pygame label Jul 7, 2024
@Starbuck5
Copy link
Member

Hello, thanks for looking at this.

This is a complex patch, and I don't really see what adding a pygame-ce specific paradigm of filtering our existing warnings does for us.

I did not anticipate pygame.debug changing any existing warnings. It instead would add a whole new category of more opiniated ones and extra information. Maybe it could use SDL_Log and change the SDL log level so we get SDL debug information too? But then again maybe the juice isn't worth the squeeze for this.

@gresm
Copy link
Contributor Author

gresm commented Jul 13, 2024

Well, this is a large patch, but it isn't complex - the logic behind it is quite simple, there are just a lot of places that needed to be updated to use the new warnings system (just renaming the PyErr_WarnEx to pgWarn and adding the urgency value).

Why I'm using what I'm using and not something like SDL warnings which have urgency? Because python warnings system was already used in the code base and I think that benefits that come from using python warnings system outweighs those of SDL warnings system (seamless integration with python code, advanced filtering system supporting regex, unit testing, etc...). An alternative implementation could create multiple filtering rules to ignore given warnings, but it would be slower (warnings that wouldn't be shown, would still need to go through the python warnings system) and harder to maintain (the necessity to keep the filter lists and updating them when a warning is added/removed/changed), so I think this is near optimal solution. That's my opinion on this matters.

Note that this PR still needs some finishing touches - pgWarn should behave the same as pygame.warn and pygame.debug isn't documented yet, I just didn't have time to do this.

@Starbuck5
Copy link
Member

Well, this is a large patch, but it isn't complex - the logic behind it is quite simple, there are just a lot of places that needed to be updated to use the new warnings system (just renaming the PyErr_WarnEx to pgWarn and adding the urgency value).

Sure, but why do we need a new warnings system?

@gresm
Copy link
Contributor Author

gresm commented Jul 20, 2024

This PR warnings system can be summarized as (omitting code for formatting warning message to include warning level):

static int
pgWarn(PyObject *category, const char *message, Py_ssize_t stack_level,
       int urgency)
{
    if (pg_warn_filter < urgency)
        return 0;

    return PyErr_WarnEx(category, message, stack_level);
}

I used incorrect wording there - this is not much "a new warnings system", but rather a thin wrapper around an existing one (and heavily used before).
This is something like @MyreMylar propesed in #2894 (comment), but using a slightly different approach at how the warnings are filtered out.

@Starbuck5
Copy link
Member

But Myre is proposing adding a new warning there, not changing existing warnings to not trigger.

@gresm
Copy link
Contributor Author

gresm commented Jul 21, 2024

Fair point, I'd like to hear your opinion on the warnings level I set for them (I've split warnings between 0, 1 and 2, where 0 and 1 are by default enabled with the intention of possibility introducing something like pygame.quiet if ever needed in the future).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New API This pull request may need extra debate as it adds a new class or function to pygame pygame.debug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

import pygame.debug
5 participants