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

Improve code style for cross-language scripting examples #8195

Merged
merged 1 commit into from
Nov 19, 2024

Conversation

wlsnmrk
Copy link
Contributor

@wlsnmrk wlsnmrk commented Oct 8, 2023

Updated C# and GDScript code examples in the cross-language scripting docs, to (hopefully) improve clarity and better match the respective code styles.

Specifically:

  • Replaced "foo/bar" example values with "my value" example values.
  • Used PascalCase for a C# field, per C# style.
  • Used camelCase for a C# local variable (see previous).
  • Used "var" for C# local variable declarations with clear right-hand types, per C# style.
  • Used PascalCase for a GDScript class/script variable, per GDScript style.
  • Explicitly stated the purpose of comments documenting expected output, to improve their clarity. I followed the style of similar comments in the GDScript format string docs for this.

@skyace65 skyace65 added enhancement topic:dotnet area:manual Issues and PRs related to the Manual/Tutorials section of the documentation cherrypick:4.0 cherrypick:4.1 labels Oct 8, 2023
Copy link
Member

@paulloz paulloz left a comment

Choose a reason for hiding this comment

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

Thanks for updating this page 🙏

@wlsnmrk wlsnmrk force-pushed the cross-scripting-style branch 2 times, most recently from 939aaf3 to 9f0c985 Compare October 9, 2023 20:40
Copy link
Member

@paulloz paulloz 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.

@paulloz
Copy link
Member

paulloz commented May 8, 2024

This is still waiting for a merge 🙏

@wlsnmrk wlsnmrk force-pushed the cross-scripting-style branch from 9f0c985 to e16be0a Compare May 8, 2024 16:58
@wlsnmrk
Copy link
Contributor Author

wlsnmrk commented May 8, 2024

Thanks for the review! While checking my changes, I noticed that C# lines casting arrays to object were removed in an earlier commit, but an accompanying warning block alerting users that the cast is required was left in. Any objections to my removing the warning, which no longer seems relevant/applicable?

@paulloz
Copy link
Member

paulloz commented May 8, 2024

Ooh, good spot! Yes, that warning block is no longer relevant.

* Replaced "foo"/"bar" example values with "my value".
* Changed C# public field to property and GDScript to match.
* Used PascalCase for C# property.
* Used camelCase for C# local variable.
* Used "var" for C# local variable declarations.
* Used PascalCase for GDScript class/script variable.
* Improved clarity of comments documenting expected output.
* Eliminated horizontal scroll in C# code example.
* Fixed a comma splice in the page text.
* Removed an out-of-date warning block.
@wlsnmrk wlsnmrk force-pushed the cross-scripting-style branch from e16be0a to aa223c3 Compare May 8, 2024 20:54
@wlsnmrk
Copy link
Contributor Author

wlsnmrk commented May 8, 2024

Thanks! Fixed.

@mhilbrunner mhilbrunner merged commit 128d712 into godotengine:master Nov 19, 2024
1 check passed
@mhilbrunner
Copy link
Member

Thank you! Merged. Finally. :) Oof, this took a while, apologies, somehow this PR slipped through. In the future, feel free to ping us if nothing moves for weeks.

@wlsnmrk wlsnmrk deleted the cross-scripting-style branch November 19, 2024 15:21
@mhilbrunner
Copy link
Member

Cherrypicked to 4.3 in #10347.

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:dotnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants