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

Validate that editor plugin classes require #[class(tool)] #852

Merged
merged 1 commit into from
Aug 12, 2024

Conversation

TitanNano
Copy link
Contributor

Editor plugin classes are editor only and thus require #[class(tool)] as they are not used in game

Details on how a class might be used is available in extension_api.json under classes.*.api_type (e.g. core or editor) and should be taken from there at some point. Matching the Editor prefix seems good enough for now, as it covers all the plugin classes. There is most likely no point extending the remaining editor only classes, but I haven't looked into it further.

@TitanNano TitanNano changed the title Validate that editor plugin classes require #[class(tool)] Validate that editor plugin classes require #[class(tool)] Aug 11, 2024
@GodotRust
Copy link

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

@TitanNano TitanNano force-pushed the jovan/tool_editor_classes branch from 7f5f6de to 24b88ea Compare August 11, 2024 20:08
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Related: #850

There is also the attribute key #[class(editor_plugin)] -- should we check against that, rather than tool?

@Bromeon Bromeon added quality-of-life No new functionality, but improves ergonomics/internals c: register Register classes, functions and other symbols to GDScript labels Aug 11, 2024
@TitanNano
Copy link
Contributor Author

There is also the attribute key #[class(editor_plugin)] -- should we check against that, rather than tool?

But that is specifically for registering the EditorPlugin class, right? I guess we should additionally check that it is also present when the base is equal EditorPlugin...

@TitanNano TitanNano force-pushed the jovan/tool_editor_classes branch 2 times, most recently from 2c88ea8 to 2501bf6 Compare August 11, 2024 21:00
@Bromeon
Copy link
Member

Bromeon commented Aug 11, 2024

Good point. I checked the original PR #393 and there's an interesting note:

The project will fail to compile if you try to make something an editor plugin that doesn't inherit from EditorPlugin. This is done by adding a doc-hidden constant to Inherits and trying to access it.

So there's already a validation that prevents #[class(editor_plugin)] from being used on other classes, and what you suggested is the inverse (having base=EditorPlugin but not editor_plugin).

I think later I'll entirely remove editor_plugin, since it's extra chore but logically equivalent to base=EditorPlugin. For now, you can thus ignore validations related to this key.

@Bromeon
Copy link
Member

Bromeon commented Aug 11, 2024

Maybe one point where #[class(editor_plugin)] could still be relevant is if we add intermediate classes between the most-derived Rust one and EditorPlugin, so the direct base class wouldn't be EditorPlugin anymore.

But even that could be somehow encoded into the type system. And currently such intermediate classes are not supported.

@TitanNano TitanNano force-pushed the jovan/tool_editor_classes branch from 2501bf6 to 09a2a8a Compare August 12, 2024 17:17
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@Bromeon Bromeon added this pull request to the merge queue Aug 12, 2024
Merged via the queue into godot-rust:master with commit 77500ba Aug 12, 2024
14 checks passed
@TitanNano TitanNano deleted the jovan/tool_editor_classes branch August 12, 2024 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: register Register classes, functions and other symbols to GDScript quality-of-life No new functionality, but improves ergonomics/internals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants