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

Generate docs from GDExtensions using --gdextension-docs with --doctool #91518

Merged
merged 1 commit into from May 7, 2024

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented May 3, 2024

Currently, --doctool won't generate docs for GDExtensions, only engine types.

This adds a new option (--gdextension-docs) which will generate docs only for GDExtensions used in the current project. It's intended to be paired with --doctool, similar to how the existing --gdscript-docs option works.

I'm not crazy about adding more stuff to main.cpp, but this is more-or-less in line with what we've already got there.

This is intended to be used with godot-cpp PR godotengine/godot-cpp#1374

main/main.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@paddy-exe paddy-exe left a comment

Choose a reason for hiding this comment

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

LGTM. The const could be added in the already specified section

if (!gdextension_docs) {
for (int i = 0; i < _doc_data_class_path_count; i++) {
// Custom modules are always located by absolute path.
String path = _doc_data_class_paths[i].path;
Copy link
Contributor

@paddy-exe paddy-exe May 6, 2024

Choose a reason for hiding this comment

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

Just had a look at this again. Could this also be a const?
const String path = _doc_data_class_paths[i].path;?

Copy link
Member

Choose a reason for hiding this comment

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

It's pre-existing code (only indentation changed here), but yes it could be const.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one actually can't be const because it's written to in the following two lines:

				if (path.is_relative_path()) {
					path = doc_tool_path.path_join(path);
				}

However, the name variable the next line down, could be const, I think. That said, since this PR is just indenting this, I'd prefer to leave the code in this block as is.

main/main.cpp Outdated Show resolved Hide resolved
@akien-mga
Copy link
Member

akien-mga commented May 7, 2024

Tested and it works great! See godotengine/godot-cpp#1374 (comment) for details.

Might be worth addressing the issue with the XML schema URI in this PR too while we're at it.
Or not, depending on the solution we go for. E.g. we could make it an absolute URL for all XML regardless of whether they're core APIs, modules, or extensions. This would affect all XML files so best kept for another PR.
But if we prefer to just special case extension XMLs like modules are already handled in DocTools::save_classes, this could be done here.

@dsnopek
Copy link
Contributor Author

dsnopek commented May 7, 2024

@paddy-exe @akien-mga @m4gr3d Thanks for the review!

Might be worth addressing the issue with the XML schema URI in this PR too while we're at it.
Or not, depending on the solution we go for. E.g. we could make it an absolute URL for all XML regardless of whether they're core APIs, modules, or extensions. This would affect all XML files so best kept for another PR.
But if we prefer to just special case extension XMLs like modules are already handled in DocTools::save_classes, this could be done here.

In my latest push, I added a possible solution for the XML schema URI issue. It adds a new argument (p_use_relative_schema) to DocTools::save_classes(), which will either use the current relative schema, or the absolute URL. This should be OK, since we haven't exposed DocTools::save_classes() and only use it from main.cpp.

Actually, it looks like the only place we use the p_include_xml_schema argument to DocTools::save_classes() is when generating GDScript docs (via --gdscript-docs). Maybe we could replace that argument with the p_use_relative_schema argument, and then use the absolute URL for both GDExtension and GDScript? That would be more consistent...

@dsnopek
Copy link
Contributor Author

dsnopek commented May 7, 2024

Actually, it looks like the only place we use the p_include_xml_schema argument to DocTools::save_classes() is when generating GDScript docs (via --gdscript-docs). Maybe we could replace that argument with the p_use_relative_schema argument, and then use the absolute URL for both GDExtension and GDScript? That would be more consistent...

I've gone ahead and switched to doing it this way in my latest push.

It looks like I'm the one who added the p_include_xml_schema argument anyway (in PR #76490), so I can be the one to replace it with something a little better as well. :-)

@akien-mga akien-mga merged commit ec78dde into godotengine:master May 7, 2024
16 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.

None yet

5 participants