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

Added section on common unsafe operations and their safe counterparts #8654

Merged
merged 1 commit into from
Jan 8, 2024

Conversation

AdriaandeJongh
Copy link
Contributor

In trying to learn typed GDScript and enabling all the advanced warnings, I found that there was no guidance anywhere in the docs on how to actually fix those warnings. This is a first attempt at that.

@AdriaandeJongh AdriaandeJongh changed the title add section on common unsafe operations and their safe counterparts Added section on common unsafe operations and their safe counterparts Dec 19, 2023
tutorials/scripting/gdscript/static_typing.rst Outdated Show resolved Hide resolved
tutorials/scripting/gdscript/static_typing.rst Outdated Show resolved Hide resolved
tutorials/scripting/gdscript/static_typing.rst Outdated Show resolved Hide resolved
tutorials/scripting/gdscript/static_typing.rst Outdated Show resolved Hide resolved
tutorials/scripting/gdscript/static_typing.rst Outdated Show resolved Hide resolved
tutorials/scripting/gdscript/static_typing.rst Outdated Show resolved Hide resolved
@AThousandShips AThousandShips added enhancement topic:gdscript area:manual Issues and PRs related to the Manual/Tutorials section of the documentation labels Dec 19, 2023
@AThousandShips AThousandShips requested a review from a team December 19, 2023 13:39
@adamscott
Copy link
Member

Nice PR! I would add though some subtitles for each error, making it easy to understand which part applies to which error.

@AdriaandeJongh
Copy link
Contributor Author

Nice PR! I would add though some subtitles for each error, making it easy to understand which part applies to which error.

Thanks!

I'm new to Sphinx / reST so I don't know if 'subtitles' hints at special reST markup or whether you're using subtitles figuratively ;) Perhaps you could give an example and I'll elaborate on the errors in the PR.

@AThousandShips
Copy link
Member

Subsections, which are done with ~ I think, see above with the type hints

@AdriaandeJongh
Copy link
Contributor Author

Ah, those kinds of subtitles! Added them.

@AdriaandeJongh
Copy link
Contributor Author

I just pushed a slightly more elaborate description of the examples, and changed the 'fix' of the second example. The old fix would work to get rid of the warning, but would lead to an error at runtime if the object would not be of the type it was casted into.

tutorials/scripting/gdscript/static_typing.rst Outdated Show resolved Hide resolved
tutorials/scripting/gdscript/static_typing.rst Outdated Show resolved Hide resolved
tutorials/scripting/gdscript/static_typing.rst Outdated Show resolved Hide resolved
tutorials/scripting/gdscript/static_typing.rst Outdated Show resolved Hide resolved
tutorials/scripting/gdscript/static_typing.rst Outdated Show resolved Hide resolved
@AdriaandeJongh
Copy link
Contributor Author

Thanks for the review @dalexeev – made all the changes you requested, as well as reordered the two examples, and added UNSAFE_PROPERTY_ACCESS to the UNSAFE_METHOD_ACCESS example (because separating them would lead to nearly identical examples, which I don't think would be better? Please advice.)

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.

@AdriaandeJongh Sorry for the delay and thanks for your patience! I found no factual errors, my comments contain only additions and style suggestions. @AThousandShips Could you please check the grammar and wording?

tutorials/scripting/gdscript/static_typing.rst Outdated Show resolved Hide resolved
tutorials/scripting/gdscript/static_typing.rst Outdated Show resolved Hide resolved
tutorials/scripting/gdscript/static_typing.rst Outdated Show resolved Hide resolved
tutorials/scripting/gdscript/static_typing.rst Outdated Show resolved Hide resolved
tutorials/scripting/gdscript/static_typing.rst Outdated Show resolved Hide resolved
tutorials/scripting/gdscript/static_typing.rst Outdated Show resolved Hide resolved
tutorials/scripting/gdscript/static_typing.rst Outdated Show resolved Hide resolved
@AdriaandeJongh
Copy link
Contributor Author

@AdriaandeJongh Sorry for the delay and thanks for your patience! I found no factual errors, my comments contain only additions and style suggestions. @AThousandShips Could you please check the grammar and wording?

No worries about the delay – I presume you're drowned in coding yourself and reviewing PRs and whatnot! I appreciate the thorough review. Latest commit is ready for final grammar / style check by @AThousandShips.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Wording and grammar is good

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.

@mhilbrunner mhilbrunner merged commit ed8a175 into godotengine:master Jan 8, 2024
1 check passed
@mhilbrunner
Copy link
Member

Awesome contribution, thanks for taking the time to add these docs. Also thanks everyone for the reviews! Merged. :)

@AdriaandeJongh AdriaandeJongh deleted the patch-3 branch January 8, 2024 08:45
mhilbrunner added a commit that referenced this pull request Jan 25, 2024
Added section on common unsafe operations and their safe counterparts
@mhilbrunner
Copy link
Member

Cherry-picked to 4.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:manual Issues and PRs related to the Manual/Tutorials section of the documentation enhancement topic:gdscript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants