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

Infer and allow BBcode lists [ul] in class reference generation #88900

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Feb 27, 2024

Supersedes #67037

I've had these two screenshots on my desktop for the past 2 weeks and I forgot to create this PR:

This PR allows the [ul] to be used in custom script's class reference generation. However, unlike #67037, it also infers a list based on whether or not a line starts with -.

Why like this?

  • It does not require changing all existing docs that already do this for a list
  • It is familiar syntax (this is how I'm writing this list on GitHub right now).
  • The online class reference generated by make_rst.py already does this, whether it's intentional or not.

(Only reason this is marked for 4.3 is because the prior PR was, too)

@Mickeon Mickeon added this to the 4.3 milestone Feb 27, 2024
@Mickeon Mickeon requested review from Calinou, dalexeev and a team February 27, 2024 11:16
@Mickeon Mickeon changed the title Infer and allow BBcode lists [ul] in built-in generation Infer and allow BBcode lists [ul] in class reference generation Feb 27, 2024
Co-Authored-By: RedMser <5117197+redmser@users.noreply.github.com>
@Mickeon Mickeon force-pushed the documentation-infer-list-generation branch from 394d7e3 to a9487c7 Compare February 27, 2024 11:17
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected.

image

Note that nested lists are not supported by this code though. The following in the XML:

- List
  - Nested
  - Item
    - More nested
  - Less nested
- Even less nested

Results in:

image

I don't recall seeing nested lists in the XML class reference, but it's worth keeping in mind for custom class reference from scripts and should be documented once we make a PR for godot-docs.

@Mickeon
Copy link
Contributor Author

Mickeon commented Feb 27, 2024

The moment nested lists will be supported is when the class reference will adopt the same syntax as GitHub's markdown which would be... wow.
Indeed there's no "nested" lists and I believe the docgen would catch and error if spaces are used to pad the beginning of a line (unless in a codeblock).

Out of guilt, I must note that there is a situation where the current code would break: since [codeblock] is not checked on the [ul] conversion pass, if a codeblock line were to start with - , it may break. As far as I know the class reference never does lists like this, but it is a possibility.

@dalexeev
Copy link
Member

dalexeev commented Feb 27, 2024

  • It does not require changing all existing docs that already do this for a list

I think it's better to update the docs and not add the inferring. Or this should be done not by a simple replacement, but take into account the context of [code], [codeblock], [ul] and [ol] (not implemented yet) in the EditorHelp BBCode parser.

## [codeblock]
## - A
## - B
## - C
## [/codeblock]
func _run() -> void:
    pass

  • The online class reference generated by make_rst.py already does this, whether it's intentional or not.

This is due to incomplete RST markup escaping.

@Mickeon
Copy link
Contributor Author

Mickeon commented Feb 27, 2024

I'd rather have it account the context better, then. But in all honestly the generation code is quite "spaghetti" as is and I wish it weren't so this addition could've been integrated more naturally.

@dalexeev
Copy link
Member

I'd rather have it account the context better, then.

I'm not sure if multiple ways to do the same is a good thing. RichTextLabel already supports [ul] and nested lists, unlike the new syntax. Yes, EditorHelp provides some shorthands like [param name] instead of [code]name[/code], but here you are mixing BBCode with Markdown.

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.

@Mickeon
Copy link
Contributor Author

Mickeon commented Feb 29, 2024

How comfortable would you be if this came along with some form of support for GitHub Flavoured Markdown-styled writing for the class ref?

I myself right now am heavily in support of the PR as is, assuming it is done in the least "hacky" way. I want existing documentation to be able to remain as is, and I want documentation to be easy to backport.

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.

4 participants