-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Code Quality: Added XML comments for C# source files #13358
Conversation
@yaira2 This is ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've not read everything in the PR because the more I read it, the more I question if it should be done altogether.
While I appreciate the desire to document the code clearly, the less comments a codebase can have, the better usually. A lot of comments done here are exact repetitions of the information given by the code below, except in a more verbose manner. And sometimes, it downright eludes every purpose of classes (I'm thinking for the App
class comments here)
If we add too much comments, we create a huge disconnected wall of text that will require to be maintained. It goes against some of the practices of clean coding (see https://gist.github.com/wojteklu/73c6914cc446146b8b533c0988cf8d29#comments-rules). We do not use the comments to clarify, explain an intent or warn about an issue. We add comments to add redundancy.
We do want to make the code base more user-friendly to acclimate the new comers, but we shouldn't increase disproportionately the comments for that.
With comments, more is less.
(Nonetheless, I really appreciate all the cleaning you've been doing in the codebase!)
Note: Not everything is to be thrown away though, some part of the cleaning done here is useful. It'd need to be trimmed a lot in my eyes.
/// <summary> | ||
/// Represents base action to compress archive. | ||
/// </summary> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you said in your PR, this info is a duplicate info from the class name.
I don't think it's a good practice to add those, it creates twice the same information, and increases the maintenance cost. Instead of having to only modify the name of the function, you have to also modify the comment. And comments are quite often forgettable. A function's name should be usually enough to explain their use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are going to add docs that will be generated from XML comments. So I just rushed to add comments and so some comments are being redundant. We can improve later, now this PR's purpose is creating practice to add comments on creating a new method, property, class.
/// <summary> | ||
/// Represents UI entry point for the Files app. | ||
/// </summary> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't agree, it's way more than just a UI entry point for Files. It handles more than that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand it has very less information than you'd expect; however the main purpose of this PR is creating practice to add comments as I described above.
Summary
You may notice that there're some comments that are almost the same as class name and the comments doesn't seem to provide more class information. Although I can imporove, important thing is to create practice to add comments and make Files contributors feel they have to add them when added a class, method, property.
Task Checklist
Known Issues
Following issues would be fixed in the upcoming PR.
N/A
PR Checklist
Steps To Validate Changes
N/A
Screenshots
N/A
Footnotes
If the request hasn't been agreed by the team, this work might be rejected. Make sure that you get approval from the team before opening any PR related the request. ↩
If you removed any en-US string resources, make sure that there are no references of those resources. When you add a new en-US string resources, do not make any changes on other languages' resources; Crowdin will do, instead. ↩