Skip to content

Conversation

@Buckram123
Copy link
Contributor

@Buckram123 Buckram123 commented Nov 24, 2025

Noticed that as_str() doesn't exist for ClassId. This PR changes comment for class_id() to use closest alternative

@Bromeon
Copy link
Member

Bromeon commented Nov 24, 2025

Thanks for the contribution. I do wonder if we should add a direct PartialEq<&str> impl in this case, so you could write:

// Before:
HttpRequest::class_id().to_cow_str() == "HTTPRequest"

// After:
HttpRequest::class_id() == "HTTPRequest"

This would save a string copy in some cases. I was already thinking about this for GString and StringName for a longer time, but for those it's quite a bit harder...

@Buckram123
Copy link
Contributor Author

@Bromeon Great suggestion! Let me know if you want it implemented in this PR

@Bromeon
Copy link
Member

Bromeon commented Nov 24, 2025

Let me know if you want it implemented in this PR

Generally yes, but I think we should think about this a bit and get some more input first 🙂


There is a small risk that a == encourages stringy comparisons, i.e.

if class_id == "MyClass"

instead of

if class_id == MyClass::class_id()

which is more error-prone.

Potential alternative would be a named method like is_class("...") or so, but then again that might be so niche that to_cow_str() is perfectly fine? 🤔

@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1412

@Bromeon Bromeon added quality-of-life No new functionality, but improves ergonomics/internals c: core Core components documentation Improvements or additions to documentation and removed quality-of-life No new functionality, but improves ergonomics/internals labels Nov 25, 2025
@Bromeon
Copy link
Member

Bromeon commented Nov 26, 2025

Seeing how the similar PR #1415 got a bit more involved, maybe we should address the == later, if we have actual use cases. Until then, we can already merge this. Thank you! 🙂

@Bromeon Bromeon added this pull request to the merge queue Nov 26, 2025
Merged via the queue into godot-rust:master with commit bdf89c4 Nov 26, 2025
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c: core Core components documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants