-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Fix docs comments in utility folder #7192
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
Fix docs comments in utility folder #7192
Conversation
| /// <param name="endOfRecord"> | ||
| /// this is true if end of record is reached | ||
| /// when delimiter is hit. This would be true if delimiter is NewLine | ||
| /// This is true if end of record is reached |
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 notice in some cases, you merged multiple lines to have a sentence on a single line but in others, such as here, you don't. Is there any particular reason for this?
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 have not set a goal to fix all the problems since there are too many. The main goal is to remove as many problems as possible to kontribjutery not diverted on these messages.
In this case, the IDE performs the formatting themselves when they show this pop-up comment. So deleting strings is better. On the other hand we need to see them on a screen width of 80-120 characters. Although I could miss something-I did not try to catch all the cases.
| { | ||
| /// <summary> | ||
| /// This class contains strings required for serialization for Convertto-XML | ||
| /// This class contains strings required for serialization for Convertto-XML. |
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.
Change case: ConvertTo-XML instead of Convertto-XML
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.
Fixed.
| /// <summary> | ||
| /// This cmdlet takes a Runspace object and checks to see if it is debuggable (i.e, if | ||
| /// it is running a script or is currently stopped in the debugger. If it | ||
| /// it is running a script or is currently stopped in the debugger. If it. |
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.
The period doesn't belong here; it breaks the sentence.
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.
Fixed.
| /// <summary> | ||
| /// one time initialization: acquire a screen host interface | ||
| /// by creating one on top of a file | ||
| /// One time initialization: acquire a screen host interface |
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.
Suggest One-time instead of one time.
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.
Fixed.
| /// <summary> | ||
| /// one time initialization: acquire a screen host interface | ||
| /// by creating one on top of a memory buffer | ||
| /// One time initialization: acquire a screen host interface by creating one on top of a memory buffer. |
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.
Suggest One-time instead of One time.
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.
Fixed.
| /// The flags to be set on the TraceSource | ||
| /// The flags to be set on the TraceSource. | ||
| /// </summary> | ||
| /// <value></value> |
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.
Suggest: remove the empty comments
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.
Fixed.
| /// </summary> | ||
| /// <remarks> | ||
| /// Causes subsequent calls to IsOpen to return false and calls to | ||
| /// Causes subsequent calls to IsOpen to return false and calls to. |
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.
Remove added period' it breaks the sentence.
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.
Fixed.
| /// <summary> | ||
| /// The following is the definition of the input parameter "Add". | ||
| /// Objects to be add to the list | ||
| /// Objects to be add to the list. |
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.
Suggest: change 'to be add' to ' to add'
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.
Fixed.
| /// If Exception is specified, this is ErrorRecord.ErrorDetails.Message. | ||
| /// Otherwise, the Exception is System.Exception, and this is | ||
| /// Exception.Message. | ||
| /// Otherwise, the Exception is System.Exception, and this is Exception.Message. |
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.
Suggest: I believe these two lines should be one sentence..
...this is ErrorRecord.ErrorDetails.Message; otherwise, the
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.
Fixed.
| /// If Exception is specified, this is ErrorRecord.ErrorDetails.Message. | ||
| /// Otherwise, the Exception is System.Exception, and this is | ||
| /// Exception.Message. | ||
| /// Otherwise, the Exception is System.Exception, and this is Exception.Message. |
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.
Suggest the same as above.... ErrorDetails.Message; otherwise, ...
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.
Fixed.
|
I looked at some of the changes and my primary concern is that adding a period to the end of a |
|
Unfortunately, most public comments are formal and do not do any benefits. We could remove them at all or replace them with a placeholder. Can someone replace all these formal comments with useful ones? It seems not real. So we can just remove the noisy CodeFactor issues formally. Let's do a formal review - we can't replace tons of formal comments with meaningful ones. |
|
@dantraMSFT Your comments was addresed. |
|
@iSazonov looking at some of the original comments in the code, they don't provide value other than get past the compiler error of not having a |
|
@dantraMSFT please take a look again. |
|
|
||
| /// <summary> | ||
| /// implementation for the export-csv command | ||
| /// implementation for the export-csv command. |
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.
Capitalize implementation.
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.
Fixed.
|
|
||
| /// <summary> | ||
| /// type definition to include in export | ||
| /// type definition to include in export. |
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.
Capitalize type
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.
Fixed.
|
|
||
| /// <summary> | ||
| /// handle to file stream | ||
| /// handle to file stream. |
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.
Capitalize handle
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.
Fixed.
| /// <summary> | ||
| /// Specifies the delivery notifications options for the e-mail message. The various | ||
| /// option available for this parameter are None, OnSuccess, OnFailure, Delay and Never | ||
| /// option available for this parameter are None, OnSuccess, OnFailure, Delay and Never. |
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.
change option to options (plural)
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.
Fixed.
|
|
||
| /// <summary> | ||
| /// Waits untill the window has been closed answering HelpNeeded events | ||
| /// Waits untill the window has been closed answering HelpNeeded events. |
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.
Until is mispelled.
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.
Fixed.
|
|
||
| /// <summary> | ||
| /// stopprocessing override | ||
| /// Stopprocessing override. |
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.
match the spelling of the Stopprocessing comment with the member name
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.
Fixed.
| /// Encoding optional flag | ||
| /// Encoding optional flag. | ||
| /// </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.
Remove the empty comment line.
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.
Fixed.
|
|
||
| /// <summary> | ||
| /// flag for one time initialization of the interface (columns, etc.) | ||
| /// Flag for one time initialization of the interface (columns, etc.). |
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.
use 'one-time' instead of 'one time'
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.
Fixed.
36cb6e2 to
43d3206
Compare
|
@dantraMSFT Your comment was addressed. |
dantraMSFT
left a comment
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.
There are 3 or 4 items that were missed. Search for ping)
Other than that, LGTM
|
I corrected one pass, the rest well disguised by GitHub. I suppose we can merge as is. |
PR Summary
The PR contains only style changes outside codes. It does not aim to eliminate all style issues - only reduce their number and increase the CodeFactor usefulness.
Changes are distributed by commits to make it easier to review.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature tests