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

Split generated RST for class reference based on the base type #63497

Merged
merged 1 commit into from
Nov 17, 2022

Conversation

YuriSizov
Copy link
Contributor

@YuriSizov YuriSizov commented Jul 26, 2022

Related to godotengine/godot-proposals#4543.

Splits generated RSTs by using a different file prefix based on the base type of each class (node/resource/other), plus a dedicated group for global scopes (starting with @). This works in tandem with the godot-docs PR (godotengine/godot-docs#5990) to give this kind of grouping:

chrome_2022-07-26_18-58-50

chrome_2022-07-26_18-59-21.mp4

@akien-mga
Copy link
Member

I like the idea generally, but I see a few issues:

  • "Nodes" / "Resources" / "Classes" - the last one is a weird catch-all. Nodes and Resources are classes too.
  • Doing the split based on adding prefixes means that we break URLs for all existing classes, and we'll have to change :ref:s all over the tutorials when referring to a class. Now one would need to remember if the class is a Node, a Resource or another "Class" to use the correct ref prefix.

@YuriSizov
Copy link
Contributor Author

the last one is a weird catch-all

It is, I kept it like that to avoid breaking all the links. Well, I guess the section itself can be called whatever... Objects, maybe? But I think from a casual user's perspective it's quite clear even with the current split.

we'll have to change :ref:s all over the tutorials when referring to a class.

No, internal identifiers for the classes are still the same and all the references should continue to work.

we break URLs for all existing classes

Yes, that is true. But this is for 4.0+, where a lot of old googled links break already due to renames, right? That said, if we change existing class_* prefix in files to something else, then we can generate class_* files with redirects or notes for every class.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

We discussed this in a meeting, the feature makes sense.

We decided to try to keep the existing format for the URL to avoid the issue of having to know how a given class was categorized to guess its URL - several of us tend to go to the class reference for a given class by editing the URL manually in our browser. This might also be relied upon by external tools that want to link to the Godot docs programmatically.

To preserve this feature, we decided to have make_rst.py generate the index.rst with the subcategories instead of relying on prefixes.

@YuriSizov
Copy link
Contributor Author

YuriSizov commented Nov 17, 2022

Changes done, the index.rst file is now generated alongside the rest of them, with a hardcoded list of all classes, split into groups as before.

Compared to the original version of this PR I've renamed "Classes" to "Objects", as it makes a bit more sense IMO. The rest should be functionally the same. Looks a little something like this:

chrome_2022-11-18_00-20-32

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

It looks amazing! 🎉

@akien-mga akien-mga merged commit cca9f78 into godotengine:master Nov 17, 2022
@akien-mga
Copy link
Member

Thanks!

akien-mga added a commit to godotengine/godot-docs that referenced this pull request Nov 17, 2022
Sync with godotengine/godot#63497.
From now on we also have to update `classes/index.rst` which is generated.

I.e. the update procedure is:
```
cd godot/doc
make rst
cd ../../godot-docs
rm -f classes/*
cp ../../godot/doc/_build/rst/* classes/
```
@YuriSizov YuriSizov deleted the docs-split-classref branch November 18, 2022 00:42
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.

5 participants