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

Add class reference documentation for GDExtension & GDExtensionManager #86968

Merged
merged 1 commit into from
Jan 11, 2024

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Jan 8, 2024

This PR fills in the class reference of GDExtension and GDExtensionManager. It also links to the existing tutorials.

I don't know what else to say.

More feedback and useful insight is necessary from actual GDExtension developers. A few of these descriptions are unfortunately still barren.

@Mickeon Mickeon requested a review from a team as a code owner January 8, 2024 18:51
@dalexeev dalexeev added this to the 4.3 milestone Jan 8, 2024
@dalexeev dalexeev requested a review from a team January 8, 2024 23:14
Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks for being the first person brave enough to attempt to document these classes :-)

doc/classes/GDExtension.xml Outdated Show resolved Hide resolved
doc/classes/GDExtension.xml Outdated Show resolved Hide resolved
doc/classes/GDExtension.xml Outdated Show resolved Hide resolved
</description>
</method>
<method name="initialize_library">
<return type="void" />
<param index="0" name="level" type="int" enum="GDExtension.InitializationLevel" />
<description>
Initializes the library bound to this GDextension.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe:

Suggested change
Initializes the library bound to this GDextension.
Initializes this GDExtension at the given initialization level.

This should probably also have a note saying that developers shouldn't call this under normal circumstance, it will be handled by GDExtensionManager.

I'm actually not entirely sure why we've exposed this at all? It's especially weird considering we haven't exposed deinitialize_library(), its paired method.

Copy link
Contributor Author

@Mickeon Mickeon Jan 9, 2024

Choose a reason for hiding this comment

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

I'm actually not entirely sure why we've exposed this at all? It's especially weird considering we haven't exposed deinitialize_library(), its paired method.

It may be worth discussing this with the rest of the GDExtension team. There have been times before where Reduz and/or others accidentally exposed methods to the documentation & scripting language "just in case" (hence the name of the PR).
After a lot of time has passed, only then you can tell there's a flaw in the API when struggling to write documentation for these.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've added an item to the agenda of tonight's GDExtension meeting, to discuss if this method should even be bound in the first place

doc/classes/GDExtension.xml Show resolved Hide resolved
doc/classes/GDExtension.xml Outdated Show resolved Hide resolved
doc/classes/GDExtension.xml Outdated Show resolved Hide resolved
doc/classes/GDExtensionManager.xml Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the reduz-mystery-meat branch 2 times, most recently from 11406b0 to c07ac11 Compare January 9, 2024 00:34
@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 9, 2024

Addressed all of the above feedback and suggestions. Thank you very much for the insight

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Looks good otherwise

doc/classes/GDExtension.xml Outdated Show resolved Hide resolved
doc/classes/GDExtensionManager.xml Outdated Show resolved Hide resolved
doc/classes/GDExtension.xml Outdated Show resolved Hide resolved
@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 9, 2024

Thank you, I'm very happy to have this as soon as possible.

@akien-mga akien-mga requested a review from dsnopek January 9, 2024 10:02
Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks!

While I personally think we should probably unbind GDExtension::initialize_library(), and hence don't need docs for it, that doesn't necessarily need to hold up this PR.

This looks great!

@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 10, 2024

Accepted the above suggestion

@akien-mga akien-mga merged commit ea83a12 into godotengine:master Jan 11, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants