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

glfw: Eliminate Error.InvalidValue #101

Closed
InKryption opened this issue Nov 26, 2021 · 2 comments · Fixed by #104
Closed

glfw: Eliminate Error.InvalidValue #101

InKryption opened this issue Nov 26, 2021 · 2 comments · Fixed by #104
Labels
Milestone

Comments

@InKryption
Copy link
Contributor

Before any work is done to progress #96, I think it wise to first mop up the errors we can potentially get rid of; that's already done for the now defunct Error.InvalidEnum, but that remains to be done for Error.InvalidValue.

This issue can serve to track discussion on that effort, but the main reason I'm opening this is because I've hit a roadblock, and would like to hear some feedback: how to deal with Joystick.updateGamepadMappings?
The GLFW call does some parsing of the passed string, and can set 'InvalidValue' if it encounters anythign unexpected, which makes preventing this error condition quite a bit more complex.

One option is to try and mimic GLFW's parsing to pre-emptively assert that the format is wrong, and perhaps deliver a useful error message to inform the caller of what's gone wrong; the other option is to simply leave the Error.InvalidValue code for this function, and allow it to be unique to it, and any other functions that may do parsing that we can't pre-empt in a maintainable fashion, which, while it doesn't allow us to eliminate the error altogether as hoped, would still be more or less made okay by error denormalization.

@slimsag
Copy link
Member

slimsag commented Nov 26, 2021

Thanks for raising this.

One option is to try and mimic GLFW's parsing to pre-emptively assert that the format is wrong, and perhaps deliver a useful error message to inform the caller of what's gone wrong

I think this would be hard to maintain in the long-term. If we did go that route, I'd prefer we instead add some validation logic to GLFW's API and then everyone who uses GLFW can benefit from it.

the other option is to simply leave the Error.InvalidValue code for this function, and allow it to be unique to it, and any other functions that may do parsing that we can't pre-empt in a maintainable fashion, which, while it doesn't allow us to eliminate the error altogether as hoped, would still be more or less made okay by error denormalization.

I think that this is the most reasonable approach. I would also say InvalidValue doesn't need to be part of the broader Error type. We could potentially rename it to ParseError too? an inline error{ ParseError } or similar

@InKryption
Copy link
Contributor Author

I agree, trying to mimic GLFW would add too much maintenance for too little benefit; felt I should put it on the table anyhow. You do make a good point that perhaps we could propose upstream for GLFW to publicize their internal 'parseMappings' function. Put a pin in that.

I'll look at making the changes whenever I get time today or tomorrow, cheers.

@slimsag slimsag added this to the Mach 0.1 milestone Mar 19, 2022
@slimsag slimsag added the glfw label Aug 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants