Skip to content

Conversation

@iSazonov
Copy link
Collaborator

@iSazonov iSazonov commented Jun 27, 2018

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.

  • Add final dot in xml doc comments.
  • Start comments with capical letter.
  • Remove empty lines in xml doc comments.
  • Add new line at EOF.
  • Remove unneeded comments.
  • Some minor fixes.

PR Checklist

/// <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
Copy link
Contributor

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?

Copy link
Collaborator Author

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.
Copy link
Contributor

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

Copy link
Collaborator Author

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.
Copy link
Contributor

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.

Copy link
Collaborator Author

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
Copy link
Contributor

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.

Copy link
Collaborator Author

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.
Copy link
Contributor

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.

Copy link
Collaborator Author

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>
Copy link
Contributor

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

Copy link
Collaborator Author

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.
Copy link
Contributor

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.

Copy link
Collaborator Author

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.
Copy link
Contributor

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'

Copy link
Collaborator Author

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.
Copy link
Contributor

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

Copy link
Collaborator Author

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.
Copy link
Contributor

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, ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

@SteveL-MSFT
Copy link
Member

I looked at some of the changes and my primary concern is that adding a period to the end of a

comment doesn't make it a sentence. Many of the existing comments are just clauses and not complete sentences and adding the period addresses the CodeFactor rule, but makes the comment worse. If we want to address those, we should have better comments that are complete sentences.

@iSazonov
Copy link
Collaborator Author

iSazonov commented Jun 29, 2018

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.

@iSazonov
Copy link
Collaborator Author

@dantraMSFT Your comments was addresed.

@SteveL-MSFT
Copy link
Member

@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 <summary/>. It seems that we would eventually want good comments so that we can generate SDK docs from the code, but that is a non-trivial task to review all the comments and make them useful and complete sentences. Perhaps we should defer that as a separate issue.

@anmenaga
Copy link

@dantraMSFT please take a look again.


/// <summary>
/// implementation for the export-csv command
/// implementation for the export-csv command.
Copy link
Contributor

Choose a reason for hiding this comment

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

Capitalize implementation.

Copy link
Collaborator Author

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Capitalize type

Copy link
Collaborator Author

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Capitalize handle

Copy link
Collaborator Author

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.
Copy link
Contributor

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)

Copy link
Collaborator Author

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Until is mispelled.

Copy link
Collaborator Author

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.
Copy link
Contributor

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

Copy link
Collaborator Author

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>
///
Copy link
Contributor

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.

Copy link
Collaborator Author

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.).
Copy link
Contributor

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'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

@iSazonov iSazonov force-pushed the fix-docs-comments-in-utility branch from 36cb6e2 to 43d3206 Compare July 16, 2018 04:40
@iSazonov
Copy link
Collaborator Author

@dantraMSFT Your comment was addressed.

Copy link
Contributor

@dantraMSFT dantraMSFT left a 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

@iSazonov
Copy link
Collaborator Author

I corrected one pass, the rest well disguised by GitHub. I suppose we can merge as is.
Thanks for the review @dantraMSFT !

@iSazonov iSazonov merged commit a378615 into PowerShell:master Jul 17, 2018
@iSazonov iSazonov deleted the fix-docs-comments-in-utility branch July 17, 2018 05:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants