Add code sample language parity check to make_rst.py
#86971
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I love this, I think it can be very helpful. Although I think the bigger problem is that the C# example exists but is outdated.
C# types with notable differences are listed in in
The problem with these types is that often the methods themselves have a different name or are in a completely different type, so adding an example under a function named after Core/GDScript will look weird. For example, the Array type has an This is one of the reasons why users ask for a dedicated C# class reference (see godotengine/godot-docs#6596). I don't want to go too off-topic, I just wanted to illustrate that this is a complicated issue so I think it may be reasonable to exclude Variant types. These types are documented in the C# source code. |
Makes sense. What do we want to do with |
All Variant types are manually defined in C# or use a BCL type except for Object which is partially generated. Object is called Some of the Variant types, even though they are manually defined, don't differ too much form Core's API. In general, if the Variant type is not listed in For the parity check we can just exclude all Variants except Object. I think that'd be good enough so we don't have to maintain a list.
I don't know. I don't mind including C# samples even if they look a bit odd and aren't very discoverable, I just wouldn't make it a requirement. Specially if this parity check is enforced in CI in the future. |
|
This is going to be very useful, thanks 🙏 |
YuriSizov
force-pushed
the
makerst-parity-check
branch
2 times, most recently
from
e3054e1 to
cb136c5
Compare
|
Removed all Variant types except for 176 code samples failed parity check. Use --verbose to get more information |
YuriSizov
force-pushed
the
makerst-parity-check
branch
from
cb136c5 to
bba3c70
Compare
akien-mga
approved these changes
Jan 16, 2024
akien-mga
changed the title
make_rst.py
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds a non-blocking validator to
make_rst.pywhich can report if codeblocks are missing parity between GDScript and C# samples. It's greedy and only ignores global contexts (which are basically GDScript-only methods). We may want to exclude Variant types as well, but I'm not confident that we should. There are differences in those types, but they should probably still be documented with correct samples. cc @raulsntos though.The report is shallow by default:
But using the new
--verboseflag you can see it in its entirety:Here's how it looks colorized:
The purpose of this is to provide a tool for documentation contributors to quickly find pieces of the docs missing C# samples. I don't think this is something we can currently enforce on the CI, but maybe one day, after increasing completion and adjusting the settings of this validator.
Inspired by Juan's tweet from a few days ago.