Skip to content

Fix (no base) showing up in all the docs #229

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

Merged
merged 1 commit into from
Apr 14, 2023

Conversation

lilizoey
Copy link
Member

@lilizoey lilizoey commented Apr 13, 2023

We were setting class_name to be (no base) in the PropertyInfo for all types by default. However that makes godot show the class_name in return values and arguments.

I dont know if there's a good way to test this? It would involve us checking what documentation is generated by godot which doesn't seem like something we can easily do.

closes #159

@lilizoey
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request Apr 13, 2023
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! 🙂

I don't think we need both a default value and Option -- there should be only one way to represent the "absent" state. Since Godot interprets the empty string as "there is no class name" (correct me if I'm wrong), the default value is more natural, and we don't need Option at all.

@@ -13,7 +13,7 @@ use crate::obj::GodotClass;

/// Utility to construct class names known at compile time.
/// Cannot be a function since the backing string must be retained.
#[derive(Eq, PartialEq, Hash, Clone, Debug)]
#[derive(Default, Eq, PartialEq, Hash, Clone, Debug)]
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a line that says that the default value (empty string) represents no class name.

Alternatively, there could be an explicit function ClassName::none().

@bors
Copy link
Contributor

bors bot commented Apr 13, 2023

try

Build succeeded:

@Bromeon
Copy link
Member

Bromeon commented Apr 13, 2023

About the test, a random idea would be to check Godot's reflection APIs, such as Object.get_method_list().

The args is only documented to be an array of dictionaries; its keys are not specified. So the doc string might or might not be part of it -- I guess we need to try.

Otherwise, it's also totally fine if we don't test everything programmatically 🙂

@lilizoey
Copy link
Member Author

About the test, a random idea would be to check Godot's reflection APIs, such as Object.get_method_list().

The args is only documented to be an array of dictionaries; its keys are not specified. So the doc string might or might not be part of it -- I guess we need to try.

Otherwise, it's also totally fine if we don't test everything programmatically slightly_smiling_face

I dont think it'll help much, as we want to make sure that the method info we output is rendered properly. but args is just an array of dictionaries that will mirror the info back at us (also the dictionary's keys are documented to match https://docs.godotengine.org/en/stable/classes/class_object.html#class-object-method-get-property-list), but wont give us info about how the documentation page renders it.

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.

True; I was actually looking for a method that would return something like a doc string, but couldn't find any.

The method list was the 2nd best option, but you're right that it might just give us what we enter. This would still test that within gdext, we end up passing the right value for class names, but this is not really giving us anything at the current complexity.

Comment on lines 22 to 27
/// In Godot, an empty `StringName` in a place that expects a class name, means that there is no class.
pub fn none() -> Self {
Self::default()
}
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I meant "alternatively" as in "not providing default".

This avoids accidental implicit construction (we also don't need 2 methods doing the same thing here).

Suggested change
/// In Godot, an empty `StringName` in a place that expects a class name, means that there is no class.
pub fn none() -> Self {
Self::default()
}
/// In Godot, an empty `StringName` in a place that expects a class name, means that there is no class.
pub fn none() -> Self {
Self {
backing: StringName::default(),
}
}

@lilizoey lilizoey added bug c: register Register classes, functions and other symbols to GDScript labels Apr 14, 2023
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!

bors r+

@bors
Copy link
Contributor

bors bot commented Apr 14, 2023

Build succeeded:

@bors bors bot merged commit 885bb91 into godot-rust:master Apr 14, 2023
@lilizoey lilizoey deleted the fix/no-base branch April 16, 2023 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug c: register Register classes, functions and other symbols to GDScript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Variant Parameter and Return Type Shows Up as (no base)
2 participants