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# XML documentation to core C# math types #40218

Merged
merged 1 commit into from
Jul 14, 2020

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Jul 8, 2020

Wow, this took about 20 hours! It seems that now about a fifth of the total lines are documentation, judging by cat * | wc -l. So far I've only done math types, I will do another PR for the rest of the files. This is part of #39703.

A lot of this documentation was taken from the core documentation, but while making this I found many areas where the grammar of the documentation could be improved, and also I discovered a few parts of the documentation that are just simply wrong.

While making this, I also added a few minor changes, including adding missing braces, simplifying/fixing code in a few places, and renaming arguments to be more descriptive. I also removed Bounce and Reflect from Vector2i because they are not methods that make sense in an integer context. I suppose I could split this into another PR, but it's much easier to... not do that.

For both of the above paragraphs, I plan to do a pass over the core files in future PRs, fixing minor API inconsistencies and updating the documentation to be more descriptive, by taking changes from this PR.

Copy link
Contributor

@neikeq neikeq left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this huge work!

end.z = point.z;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really like the idea of enforcing braces in C# where the opener goes on a new line. We don't have to follow the C++ code style here. Or do we actually enforce braces in the C# code style and I forgot about it?

This is not a blocker, but I would prefer without it if possible (unless it's from our C# code style and I forgot).

Copy link
Member Author

Choose a reason for hiding this comment

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

The C# style guide doesn't say that braces should always be enforced, but it does say how they should be placed, all examples have braces, and I would totally be in favor of adding this to the style guide. It is more readable in my opinion, and I think being consistent with the C++ code is a good thing.

Copy link
Member

Choose a reason for hiding this comment

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

2 lines vs 4 lines for a short if is a tough sell with the brace style used for C# though.

I don't think the C# and C++ code styles have to be consistent per se, but I'm not opposed to it either.

https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/inside-a-program/coding-conventions shows both styles in their examples but predominantly the one with braces though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I changed some of the parts of the code back to no braces, specifically, the parts in AABB/Rect2 with repeated "return false;" statements, since I think those parts of the code need it the least.

However, I kept other instances of braces, since I think it's more readable and should be the default choice.

@aaronfranke aaronfranke force-pushed the mono-docs branch 3 times, most recently from 241f769 to 0f0dd1c Compare July 11, 2020 08:06
@aaronfranke
Copy link
Member Author

I did a lot of force pushes recently because I was adding the documentation to core and while doing so I noticed a few problems. However, I think the documentation in this PR is good now.

@akien-mga akien-mga merged commit 861c6c6 into godotengine:master Jul 14, 2020
@akien-mga
Copy link
Member

Thanks!

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.

3 participants