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

Add C# code to random_number_generation and fix shuffle bag example. #8072

Merged
merged 10 commits into from
Nov 20, 2024

Conversation

ssBandit
Copy link
Contributor

Changes made to shuffle bag example:

  • Replace .empty() with .is_empty()
  • More descriptive comment
  • Changed code-block into a tabbed code block

Changes made to shuffle bag example:
- Replace .empty() with .is_empty()
- More descriptive comment
- Change code-block into a tabbed code block
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.

Hello 👋 Thanks for the contribution.
I made some comment in-line. In general, I'm not sure if we should use Godot collections here, instead of system ones. I don't really see an obvious reason for the former, and we establish here to avoid it if possible. Maybe someone else can confirm/infirm that (cc @raulsntos?).

tutorials/math/random_number_generation.rst Outdated Show resolved Hide resolved
tutorials/math/random_number_generation.rst Outdated Show resolved Hide resolved
tutorials/math/random_number_generation.rst Show resolved Hide resolved
tutorials/math/random_number_generation.rst Outdated Show resolved Hide resolved
tutorials/math/random_number_generation.rst Outdated Show resolved Hide resolved
@ssBandit
Copy link
Contributor Author

Thanks for taking a look and noticing the mistakes.

The reason for using Godot collections is that in the shuffle bag example it uses .Shuffle(), and the metal example uses .Duplicate() which would be bit more difficult to do with pure system collections.
(And the documentation kinda implies the use of Godot collections for Array and Dictionary. Maybe there needs to be a disclaimer about it there.)

@raulsntos raulsntos added enhancement topic:dotnet area:manual Issues and PRs related to the Manual/Tutorials section of the documentation labels Sep 30, 2023
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

In general I think we should prefer System collections, as recommended in the documentation page that @paulloz linked.

I can see how using the Shuffle method is more convenient than using a custom solution. But I don't think it's too much code.

I think if this documentation page is meant to be a general guide to randomness, using System collections is preferable. I don't think the goal of this page is to show you how to use Shuffle specifically, but rather to explain the shuffle bag pattern (which can also be implemented without Godot's Shuffle method).

I don't think the text necessarily implies using Godot collections, the word array can be understood as C# arrays, and dictionary is also the name of the System collection. But I see your point, it's true that sometimes the text in the documentation doesn't align perfectly with the C# examples, but that's already the case in many documentation pages.

tutorials/math/random_number_generation.rst Outdated Show resolved Hide resolved
tutorials/math/random_number_generation.rst Outdated Show resolved Hide resolved
tutorials/math/random_number_generation.rst Outdated Show resolved Hide resolved
tutorials/math/random_number_generation.rst Outdated Show resolved Hide resolved
tutorials/math/random_number_generation.rst Show resolved Hide resolved
@mhilbrunner mhilbrunner requested a review from raulsntos October 6, 2023 10:54
@paulloz
Copy link
Member

paulloz commented Feb 4, 2024

@raulsntos should we finally get this merged?

Copy link
Member

@raulsntos raulsntos 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 the ping, I totally forgot about this. I took another look and found a couple things that I missed in my previous review.

tutorials/math/random_number_generation.rst Outdated Show resolved Hide resolved
tutorials/math/random_number_generation.rst Outdated Show resolved Hide resolved
tutorials/math/random_number_generation.rst Outdated Show resolved Hide resolved
tutorials/math/random_number_generation.rst Outdated Show resolved Hide resolved
tutorials/math/random_number_generation.rst Outdated Show resolved Hide resolved
tutorials/math/random_number_generation.rst Outdated Show resolved Hide resolved
tutorials/math/random_number_generation.rst Outdated Show resolved Hide resolved
Copy link
Member

@raulsntos raulsntos 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 it took me so long to review.

@ssBandit
Copy link
Contributor Author

ssBandit commented Feb 8, 2024

No problem. Thanks for the diligent review.

@tetrapod00 tetrapod00 merged commit 892322b into godotengine:master Nov 20, 2024
@tetrapod00
Copy link
Contributor

Thanks, and thanks for your patience with the long review process! Congrats on your first merged docs contribution

@ssBandit
Copy link
Contributor Author

Thank you.
Even though the one thing I was meaning to correct in the first place (empty() -> is_empty()) was fixed already by someone else 🥲
I should split commits into separate pull requests in the future.

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.

4 participants