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

Dump API docs from inline GDScript comments using --doctool --gdscript-docs PATH #76490

Merged
merged 1 commit into from
May 9, 2023

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Apr 26, 2023

Godot is now parsing inline API documentation in GDScript, in order to show it in the editor's integrated help system.

This PR allows dumping this as XML by passing --doctool --gdscript-docs PATH on the command-line, using the same XML format as --doctool normally does for the engine API documentation.

This could allow addon authors to further process the XML, for example, to create an online API reference.

I'm not 100% sure that having this in the engine is a good idea. It wouldn't be that hard to create an external tool that parsed GDScript's inline API documentation (like https://github.com/GDQuest/gdscript-docs-maker). The advantage to having it in engine is that it will exactly match what shows up in the editor's help system.

What do you think?

@dsnopek dsnopek added this to the 4.x milestone Apr 26, 2023
@dsnopek dsnopek requested a review from a team as a code owner April 26, 2023 21:47
@dsnopek dsnopek assigned mhilbrunner and Faless and unassigned mhilbrunner and Faless Apr 26, 2023
@dsnopek dsnopek requested review from mhilbrunner and Faless April 26, 2023 21:50
@groud
Copy link
Member

groud commented May 2, 2023

The feature makes sense to me.

Cc @NathanLovato for the GDquest team, if that looks good to you as your team is working on documentation tooling.

@NathanLovato
Copy link
Contributor

It does look good, though note that our tool was mostly to fill the gap for Godot 3, as having docs generated in the editor was the biggest need for us.

@dsnopek dsnopek force-pushed the dump-gdscript-docs branch 2 times, most recently from f40a332 to 734d23b Compare May 8, 2023 15:41
@dsnopek dsnopek requested a review from a team as a code owner May 8, 2023 15:41
@dsnopek dsnopek force-pushed the dump-gdscript-docs branch from 734d23b to a64137d Compare May 8, 2023 16:00
@akien-mga
Copy link
Member

Didn't review in depth, but some comments:

  • The feature makes sense to me 👍
  • --doctool doesn't seem needed at all, is it? Just godot --gdscript-docs PATH should be sufficient.
  • I'm not fond of breaking encapsulation and having code in main.cpp that depends on the GDScript module. We do have such a hack for Mono currently, so this isn't worse per se, but it's still adding more tech debt to our nasty main.cpp. The whole file requires a full overhaul if we want to make it possible for modules to define their own command line options though, so for now we might have to live with the hack.

@dsnopek
Copy link
Contributor Author

dsnopek commented May 9, 2023

Thanks for the review!!

--doctool doesn't seem needed at all, is it? Just godot --gdscript-docs PATH should be sufficient.

--doctool isn't strictly necessary, but I'm using it because:

  1. It sets some flags we need set for --gdscript-docs anyway
  2. It can set the output path if you give it an argument like --doctool PATH (like you can with normal --doctool usage). If we didn't use --doctool for this, we'd need to add another option like --gdscript-docs-output or something.
  3. It's conceptually related (ie. we're generating docs XML) and combining --doctool with another option has a precedent with --no-docbase

That said, if you'd prefer that we didn't use --doctool and instead added a --gdscript-docs-output option, I'd be fine to update the PR for that! :-)

I'm not fond of breaking encapsulation and having code in main.cpp that depends on the GDScript module. We do have such a hack for Mono currently, so this isn't worse per se, but it's still adding more tech debt to our nasty main.cpp. The whole file requires a full overhaul if we want to make it possible for modules to define their own command line options though, so for now we might have to live with the hack.

Yeah, I'm not crazy about adding more junk to main.cpp either. I did it mainly because --doctool support is already embedded in main.cpp.

It'd be great to extract it, but we'd still need a way to affect how CLI arguments are parsed depending on if GDScript is enabled (since we wouldn't want to offer a --gdscript-docs option if there is no GDScript), and so to completely remove it, we'd need a new system that somehow allowed modules to hook into CLI parsing.

Anyway, I was hoping this could sneak in for now given that it's not too much code. :-) But let me know if this is blocker, and then I'll try to think up some options.

@akien-mga
Copy link
Member

It can set the output path if you give it an argument like --doctool PATH (like you can with normal --doctool usage). If we didn't use --doctool for this, we'd need to add another option like --gdscript-docs-output or something.

I didn't notice that you actually needed two paths for this. Makes sense to use --doctool for the output path then.

Anyway, I was hoping this could sneak in for now given that it's not too much code. :-) But let me know if this is blocker, and then I'll try to think up some options.

No it's ok to proceed with the hack for now. A better system IMO depends on refactoring the whole of main.cpp, which is something I've wanted to do for a long time, but ENOTIME.

Copy link
Member

@mhilbrunner mhilbrunner left a comment

Choose a reason for hiding this comment

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

LGTM, and very useful feature.

I've seen it in use at W4 to create online docs for a Godot Addon. Having the same docs XML > Sphinx > ReadTheDocs workflow we already do but for Godot GDScript addons is potentially very cool.

I really want to see this integrated with a new version of the Asset Lib so people can get a online reference built for their addons automatically (optionally of course).

@akien-mga akien-mga modified the milestones: 4.x, 4.1 May 9, 2023
@akien-mga akien-mga merged commit 10ed1d8 into godotengine:master May 9, 2023
@akien-mga
Copy link
Member

Thanks!

@dsnopek dsnopek deleted the dump-gdscript-docs branch July 22, 2024 15:26
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