FlorianRhiem / pyGLFW

Python bindings for GLFW
MIT License
232 stars 36 forks source link

Default Error handling differs from GLFW #54

Closed FlorianLudwig closed 3 years ago

FlorianLudwig commented 4 years ago

According to GLFW developer @elmindreda (see https://github.com/glfw/glfw/issues/1121#issuecomment-574682022) errors in GLFW are not critical (as in halting the execution of the program). In pyGLFW the default behaviour is the opposite (halting execution on all errors). This makes for a bad developer experience and breaks wayland support (see mentioned issue request)

Therefore I propose to change:

ERROR_REPORTING = True

To

ERROR_REPORTING = "warn"
FlorianRhiem commented 4 years ago

Hello Florian,

pyGLFW error handling was initially modelled after PyOpenGL error handling with its ERROR_CHECKING flag. As a result of this issue, which also led to the comment you linked, I've added the 'warn' and 'log' options to the glfw.ERROR_REPORTING flag. I kept the default to True, to stay compatible with previous versions. Of course I could make this change in a major release to denote the breaking change.

I'm also not sure that False would be the best default value, if I were to change it. While it would be far more similar to GLFW's approach in C, it would also be highly unusual for Python ("Errors should never pass silently. Unless explicitly silenced." - PEP 20). The use of exceptions, the warnings module or the logging module are more typical than making the user register an error handler to find out when something failed or broke. My worry is that making the experience slightly better for developers with non-Python GLFW experiences (saving them that one line) might make the experience significantly worse for Python developers without GLFW experience (making them wonder why something is broken, as the program reports no issues).

FlorianLudwig commented 4 years ago

Hey Florian,

As a result of this issue [...]

Actually the same issue I had, which made me stumble :]

I'm also not sure that False would be the best default value, if I were to change it.

I'd say the warn would be preferable

Errors should never pass silently. Unless explicitly silenced

While I agree with the zen of python,... It seems that the word "error" in glfw means something different than what the python devs mean with it.

Also, the "explicitly silenced" is not possible as all errors are the same class so I cannot, for example:

try:
    w = glfw.create_window(640, 480, "glfw window", None, None)
except CannotFocusError:
   pass

There seem to be some work going on glfw side to improve the current situation - but I am not sure what exactly that means: https://github.com/glfw/glfw/issues/1121#issuecomment-637690869

FlorianRhiem commented 4 years ago

I agree that 'warn' seems like a better default than False as it will not silence the errors. Also, an GLFWError exception should make the error code available in a more reasonable way (i.e. as an attribute code) than just as part of the text. That way a GLFWError could be ignored using the new error codes for unavailable or unimplemented features. Additionally, ERROR_REPORTING could be set for specific error codes using a dict, though perhaps that might be a level of complexity where a user would want to implement their own error handler instead.

I'd like to leave this issue open for a short while in the hope of gathering some opinions/thoughts from other users.

FlorianRhiem commented 4 years ago

I've implemented the potential changes in feature-error-handling.

FlorianLudwig commented 4 years ago

I like the error_reporting as dict solution! Nice

FlorianRhiem commented 3 years ago

I've included the changes in master and released it as v2.0.0.