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

Update to v2025.1, bump version, and use errors #155

Merged
merged 7 commits into from
Mar 10, 2025

Conversation

mehmetoguzderin
Copy link
Contributor

First of all, thank you very much for creating these bindings! Both *-sys and *-rs crates have good organization, which makes things convenient to develop.

While iterating some Python bindings based on these Rust bindings (this https://github.com/mehmetoguzderin/shaderc-rs-py repo), the lack of version bumps and updated dependencies started causing problems due to missing enums, etc. This PR aims to address those. This update also fixes some typos and uses errors rather than options, potentially addressing the following issue, too: #154 (also the request for submodule version update in #150 issue)

I can update the PR and adjust it to make it more suitable if needed. Thank you for your consideration!

Ok(())
}
Error::InternalError(ref r) => {
if r.is_empty() {
write!(f, "internal error")
} else {
write!(f, "internal error: {r}")
write!(f, "internal error: {}", r)
Copy link
Contributor

Choose a reason for hiding this comment

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

these changes will make clippy sad

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@filnet thank you for pointing out! I've gone through these including format! etc. I hope it is better now. Please LMK if anything else would make it more suitable!

@mehmetoguzderin mehmetoguzderin requested a review from filnet March 2, 2025 12:29
Copy link
Collaborator

@antiagainst antiagainst left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for the pull request! I just have a few nits. You may also want to merge in origin/main now given I fixed a few formating and CI issues to make CI pass.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@mehmetoguzderin
Copy link
Contributor Author

@antiagainst thank you very much for the review! Indeed, making uppercase without trailing dot makes things look more consistent. I've also merged upstream to here! If there's any other update that'd make this PR more suitable, PLMK and I'll adjust! Thank you!

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@antiagainst antiagainst merged commit 9dbba1d into google:main Mar 10, 2025
11 checks passed
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

3 participants