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

Align raylib-go ExportImage with C Library Behavior #342

Merged
merged 2 commits into from
Jan 16, 2024

Conversation

shellfu
Copy link
Contributor

@shellfu shellfu commented Jan 15, 2024

The original raylib C library's ExportImage function returns a boolean to indicate the success or failure of the export operation. This behavior was missing in the raylib-go implementation, which provided no return value, thereby limiting error handling capabilities.

This commit updates the ExportImage function in raylib-go to return a boolean, aligning it with its C counterpart and enabling idiomatic Go error handling. The change includes updates to the function and tests to reflect the new return type. This enhancement increases robustness and clarity in error handling for Go developers using raylib-go.

The test suite has been updated and run to ensure the correct functioning of the modified ExportImage function, with results confirming the expected behavior in both successful and unsuccessful scenarios.

  • Daniel "ShellFu" Kendrick

The original raylib C library's ExportImage function returns a boolean to
indicate the success or failure of the export operation. This behavior was
missing in the raylib-go implementation, which provided no return value,
thereby limiting error handling capabilities.

This commit updates the ExportImage function in raylib-go to return a boolean
or an error, aligning it with its C counterpart and enabling idiomatic Go
error handling. The change includes updates to the function and tests to
reflect the new return type. This enhancement increases robustness and clarity
in error handling for Go developers using raylib-go.

The test suite has been updated and run to ensure the correct functioning of
the modified ExportImage function, with results confirming the expected
behavior in both successful and unsuccessful scenarios.

- Daniel "ShellFu" Kendrick
@shellfu
Copy link
Contributor Author

shellfu commented Jan 15, 2024

Closes #341

@gen2brain
Copy link
Owner

You are removing some newly added functions. Also, there is nothing special in ExportImage, so it warrants a new test file just for that. This is also a rare case where the test can work because it is not needed to run on the main thread.

@shellfu
Copy link
Contributor Author

shellfu commented Jan 15, 2024

just did a fresh clone, ill check again to make sure I have the same file. I added a test for what I changed but if you don't want any tests that is fine.

@gen2brain
Copy link
Owner

Thanks, I prefer not to add a test.

@shellfu
Copy link
Contributor Author

shellfu commented Jan 15, 2024

Test removed and two functions added back in.

Additionally, as a suggestion from a contributor's perspective, it might be beneficial to include a CONTRIBUTORS.md file in the repository. This would offer guidance on your preferences and expectations for contributions, ensuring everyone is on the same page. It’s a great way to maintain clarity and consistency in the project.

Lastly, while I understand this project primarily serves as a binding for the C library, I believe maintaining an idiomatic Go feel in its usage and structure of the packages themselves would greatly benefit the user experience. This approach could significantly enhance the package intuitiveness for Go developers.

Thank you for considering these ideas. Looking forward to your thoughts on this.

@gen2brain gen2brain merged commit 49aab27 into gen2brain:master Jan 16, 2024
7 checks passed
@gen2brain
Copy link
Owner

Merged, thanks. Sorry, I don't have time to write a docs or similar. If you have some proposals feel free to open discussion/issue.

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.

2 participants