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

In make_rst.py, include the parent class in 'Inherits:' even if it is not known. #97337

Merged
merged 1 commit into from
Sep 23, 2024

Conversation

Ivorforce
Copy link
Contributor

@Ivorforce Ivorforce commented Sep 22, 2024

With this PR, in the rare case that a parent class has not yet been documented, it will still appear in "Inherits:", as in:

SCR-20240922-selz

This is related to godotengine/godot-proposals#10733: When the script is used for a gdextension codebase, the script will not be aware of the parent class. This mitigates the issue of an empty "Inherits:" notation (the current state):

SCR-20240922-sezk

Additionally, an error will now be printed through make_type, where this mistake was silently ignored before.

Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

This seems okay. I'm not sure who else can take a look at make_rst.py, though, just to ensure it doesn't rarely behave weird occasionally.

@dalexeev
Copy link
Member

dalexeev commented Sep 23, 2024

I don't fully understand the change. If RefCounted is displayed instead of an unknown class, I agree that it can be considered a bug. But the second picture doesn't look right either. I don't think the empty "Inherits:" list makes sense, we should either hide it or display a placeholder like <UnknownClass> (or the parent class name as text, without a hyperlink).

In the rare case that a parent class has not yet been documented

Note that this is not a normal situation. If a class is exposed, its parent must be too. There should be no invalid or missing links to the parent class.

@Ivorforce
Copy link
Contributor Author

I ran the tool against the current codebase, at least right now it doesn't do anything different.

@dalexeev
Copy link
Member

dalexeev commented Sep 23, 2024

I ran the tool against the current codebase, at least right now it doesn't do anything different.

Yes, but we are talking about how the code should work in such a situation. You proposed reStructuredText generation for user docs and want to document the use of make_rst.py for this purpose. So we can't use the argument "it doesn't do anything different for the current Godot codebase", given that the PR is specifically about a situation that does not happen normally in Godot. But it can happen by mistake or in third-party projects using make_rst.py.

@Ivorforce
Copy link
Contributor Author

Ivorforce commented Sep 23, 2024

@dalexeev Maybe you misunderstand. This PR changes from the second picture to the first picture. Where before, you had an empty Inherits: part, with this PR you will at least have the name of the parent class, plus an error in the log.

A better solution for gdextension would involve referencing against and linking the official godot docs, but that's a lot more work. This is a decent hotfix for an odd situation.

@dalexeev
Copy link
Member

By the way, NumDot Docs are not displayed correctly. Your classes are obviously not Variant types, this can be confusing for NumDot users. As far as I understand, the whole problem is that your class reference does not have Godot classes, so you get incorrect inheritance and missing links (see Variant and int).

If NDArray inherits RefCounted, then the picture is absolutely correct, you just don't have RefCounted documentation, just like you don't have Variant and int:

Ideally, you should modify make_rst.py to redirect links to Godot documentation. But this can be a lot of work, which probably won't be appropriate for our make_rst.py / godot-docs.

@Ivorforce
Copy link
Contributor Author

Ivorforce commented Sep 23, 2024

@dalexeev Yes, both are addressed in the latest versions of the docs: https://numdot.readthedocs.io/en/latest/classes/class_nd.html

Basically i used the changes to make_rst.py proposed in this PR to create the version (viewable on latest) with the fixed inherits. That's why i proposed them!

Ideally, you should modify make_rst.py to redirect links to Godot documentation. But this can be a lot of work, which probably won't be appropriate for our make_rst.py / godot-docs.

I agree, but this is out of scope for the near future. Just seeing the name of the parent is 'good enough' for now.

Copy link
Member

@dalexeev dalexeev 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 to me. Thanks and sorry for the misunderstanding!1

Footnotes

  1. I missed the text "With this PR," and that the pictures were in reverse order. To avoid such questions in the future, you could use "Before" and "After" captions. Please note that many contributors are not native English speakers.

@dalexeev dalexeev modified the milestones: 4.x, 4.4 Sep 23, 2024
@Ivorforce
Copy link
Contributor Author

Ivorforce commented Sep 23, 2024

Yep, sorry. I agree this PR wouldn't make sense with the reverse changes. I'll structure it better next time!

@akien-mga akien-mga merged commit d5aadc3 into godotengine:master Sep 23, 2024
19 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.

5 participants