Refactor some export related stuff by lilizoey · Pull Request #311 · godot-rust/gdext (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation10 Commits3 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Split off from #309
- Renames
godot-core::exportintogodot-core::property, in preparation for Distinguish between exported and non-exported properties #309 - Convert
bailanderrorin godot-macros to macros, so we can do for instancebail!(tt, "something {key} something")instead ofbail(format!("something {key} something"), tt) - Moves property/export related stuff out of
derive_godot_class.rsand into its own module - Moves
KvParserout ofutil.rsand into its own module
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for splitting the PR, and into separate commits 👍
Should we not use godot::bind::property instead of godot::property? It was wrong already and I'm not asking to rework the whole module structure; it could probably be adjusted in godot/lib.rs via re-imports.
It's a bit a pity that Git cannot detect "splits" so we only see util.rs -> kv_parser.rs and not also util.rs -> util/mod.rs...
Since I cannot easily track those changes: did you modify anything inside code that was moved via remove+add?
Comment on lines 73 to 79
| /// Given an impl block for a trait, returns whether that is an impl |
|---|
| /// for a trait with the given name. |
| /// |
| /// That is, if `name` is `"MyTrait"`, then this function returns true |
| /// if and only if `original_impl` is a declaration of the form `impl |
| /// MyTrait for SomeType`. The type `SomeType` is irrelevant in this |
| /// example. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is new code since Git doesn't detect it as rename, but I'll mention it anyway:
Lines should be wider. The horizontal line above is a good guideline -- you don't have to use the full width and I don't want to start counting characters, but at the moment this block uses less than half 🙂 maybe 2/3-3/4 is ok.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah this isn't new code, i can change it though
Any feedback on this?
Should we not use
godot::bind::propertyinstead ofgodot::property? It was wrong already and I'm not asking to rework the whole module structure; it could probably be adjusted ingodot/lib.rsvia re-imports.
Apart from that, should be good 🙂
Any feedback on this?
Should we not use
godot::bind::propertyinstead ofgodot::property? It was wrong already and I'm not asking to rework the whole module structure; it could probably be adjusted ingodot/lib.rsvia re-imports.Apart from that, should be good slightly_smiling_face
What exactly are you suggesting? that we should re-export godot-core::property as godot::bind::property? because we currently do re-export the symbols from property in godot::bind and use those.
Not re-export, move.
Each symbol should appear in exactly 1 public module, and possibly the prelude. The status quo violates this no-duplicate rule in a few places, which was mostly due to fast-paced development without intermediate cleanups, but we should avoid creating more tech-debt here.
Since all this functionality is related to the binding/exporting/offering of the Rust API to Godot, it should ideally go into module bind. The name can of course be discussed, but I'd prefer that at a later time 🙂
- move
godot-macros/src/derive_godot_class.rsandgodot-macros/src/util.rsinto their own folders
- Split
KvParserout ofutil/mod.rs - Move
godot::propertyintogodot::bind::property
Not re-export, move.
Each symbol should appear in exactly 1 public module, and possibly the
prelude. The status quo violates this no-duplicate rule in a few places, which was mostly due to fast-paced development without intermediate cleanups, but we should avoid creating more tech-debt here.Since all this functionality is related to the binding/exporting/offering of the Rust API to Godot, it should ideally go into module
bind. The name can of course be discussed, but I'd prefer that at a later time slightly_smiling_face
ok i made it so that godot_core::property is now only exported in godot::bind::property, except for Export and TypeStringHint which are also exported in godot::prelude.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build succeeded!
The publicly hosted instance of bors-ng is deprecated and will go away soon.
If you want to self-host your own instance, instructions are here.
For more help, visit the forum.
If you want to switch to GitHub's built-in merge queue, visit their help page.
bors bot added a commit that referenced this pull request
309: Distinguish between exported and non-exported properties r=Bromeon a=lilizoey
Built on top of #311
Split #[export] into #[var] and #[export]
var is now for creating properties, and export for exporting to the editor. export implies var, meaning that the following two field declarations are equivalent:
#[var]
#[export]
field: SomeType
// and
#[export]
field: SomeTypeFieldVar and FieldExport are now the types used to store the information about the var and export attributes.
Add support for the various export annotation
things like @export_range and such
Add a list parser to godot-macros
ListParser is used to parse values from the KvParser into ordered lists of items.
We could've reused KvParser, but that would require for instance ``@export_range(0.0, 10.0, or_greater) to look something like:
#[export(range = (min = 0.0, max = 10.0, or_greater))]
Which to me doesn't seem ideal. I dont think we want people to look up the attribute names of the fields of an export annotation to figure out how to rewrite it in rust. With the list parser the above can instead just become:
#[export(range = (0.0, 10.0, or_greater))]
Split up Export trait into Property and Export
Property has a setter and getter function for generating setters/getters. In addition to an associated type for the intermediate value used to pass objects to and from godot.
Export is now only for default export info, and functioning as a marker trait for exportable types.
Add in functions to create ExportInfo structs based on each export annotation
Used as a typed api to ensure that the user passes in the right values and can in the future be used by the builder api too to ensure compatibility between the two APIs.
These are all placed in godot-core::property::export_info_functions.
Add ExportableObject for exportable GodotClass objects.
An object can be exported if it inherits from either Node or Resource. Unfortunately this means there is no really nice way to create a bound for that using the existing traits. So i made ExportableObject, which is automatically implemented by all objects inheriting from one of those two classes.
fixes #227
Co-authored-by: Lili Zoey lili.andersen@nrk.no
bors bot added a commit that referenced this pull request
309: Distinguish between exported and non-exported properties r=Bromeon a=lilizoey
Built on top of #311
Split #[export] into #[var] and #[export]
var is now for creating properties, and export for exporting to the editor. export implies var, meaning that the following two field declarations are equivalent:
#[var]
#[export]
field: SomeType
// and
#[export]
field: SomeTypeFieldVar and FieldExport are now the types used to store the information about the var and export attributes.
Add support for the various export annotation
things like @export_range and such
Add a list parser to godot-macros
ListParser is used to parse values from the KvParser into ordered lists of items.
We could've reused KvParser, but that would require for instance ``@export_range(0.0, 10.0, or_greater) to look something like:
#[export(range = (min = 0.0, max = 10.0, or_greater))]
Which to me doesn't seem ideal. I dont think we want people to look up the attribute names of the fields of an export annotation to figure out how to rewrite it in rust. With the list parser the above can instead just become:
#[export(range = (0.0, 10.0, or_greater))]
Split up Export trait into Property and Export
Property has a setter and getter function for generating setters/getters. In addition to an associated type for the intermediate value used to pass objects to and from godot.
Export is now only for default export info, and functioning as a marker trait for exportable types.
Add in functions to create ExportInfo structs based on each export annotation
Used as a typed api to ensure that the user passes in the right values and can in the future be used by the builder api too to ensure compatibility between the two APIs.
These are all placed in godot-core::property::export_info_functions.
Add ExportableObject for exportable GodotClass objects.
An object can be exported if it inherits from either Node or Resource. Unfortunately this means there is no really nice way to create a bound for that using the existing traits. So i made ExportableObject, which is automatically implemented by all objects inheriting from one of those two classes.
fixes #227
Co-authored-by: Lili Zoey lili.andersen@nrk.no
Labels
Register classes, functions and other symbols to GDScript
No new functionality, but improves ergonomics/internals