GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Remove empty xml comment lines by iSazonov · Pull Request #7356 · PowerShell/PowerShell

Conversation

Ilya (iSazonov) commented Jul 25, 2018

Copy link

Copy Markdown

Collaborator

PR Summary

Please review commit by commit.

The PR only remove empty public comment XML lines. Please don't request other changes.
The changes were made in Notepad++ by regex.
Some files (~3) was edited manually.
Files (~10) with single changes

/// <summary>
///
/// </summary>

with

/// <summary>
/// </summary>

was reverted because there is no benefits.

PR Checklist

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
Change is not breaking
Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.
User-facing changes
- Not Applicable
- OR
- User-facing Documentation needed
  - Issue filed - Issue link:
Testing - New and feature
- Not Applicable or can only be tested interactively
- OR
- Make sure you've added a new test if existing tests do not effectively test the code changed
  - Add [feature] if the change is significant or affects feature tests

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) added 18 commits

July 25, 2018 11:15


          Remove 1

c2bcf8b


          Remove 2

0d3b736


          Remove 3

5778a72


          Remove 4

d606dd9


          Remove 5

e81665a


          Remove 6

77c2547


          Remove 7

ab0032e


          Remove 8

fe9aafe


          Remove 9

4b0c962


          Remove 10

1888b06


          Remove 11

910049f


          Remove 12

fb17867


          Remove 13

52aecbf


          Remove 14

de13652


          Remove 15

a8b06f4


          Remove 16

88bfe00


          Remove 17

a5247ed


          Remove 18

0f75692

Ilya (iSazonov) self-assigned this

Jul 25, 2018

Ilya (iSazonov) requested review from Steve Lee (SteveL-MSFT), Travis Plunk (TravisEz13), Aditya Patwardhan (adityapatwardhan) and Dan Travison (dantraMSFT)

July 25, 2018 07:21

Ilya (iSazonov) requested review from Bruce Payette (BrucePay), Francisco Gamino (Francisco-Gamino), James Truher (JamesWTruher), Paul Higinbotham (PaulHigin), Andrew (anmenaga), Dongbo Wang (daxian-dbw) and Mike Richmond (mirichmo) as code owners

July 25, 2018 07:21

Steve Lee (SteveL-MSFT) requested changes

Jul 27, 2018

View reviewed changes

Steve Lee (SteveL-MSFT) left a comment

Copy link

Copy Markdown

Member

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 spent a bit of time up through the 4th commit. My main feedback is that where there is content between xml tags, it seems we should preserve the whitespace as it makes reading the comment easier. I'm ok with removing the whitespace between different xml tags.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/Microsoft.Management.Infrastructure.CimCmdlets/NewCimSessionOptionCommand.cs

		/// can subsequently use to create one or more CimSession connections. The
		/// options object holds the CIM Session information that is less commonly set
		/// and used by the IT Pro, and most commonly defaulted.
		///

Steve Lee (SteveL-MSFT) Jul 27, 2018

Copy link

Copy Markdown

Member

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.

It seems that this line makes it more readable as there's two paragraphs

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) Jul 27, 2018

Copy link

Copy Markdown

Collaborator Author

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 many such places. Since I used automatic replacement, it is really impossible to fix it everywhere.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Steve Lee (SteveL-MSFT) Jul 27, 2018

Copy link

Copy Markdown

Member

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.

Is it possible to craft a regex so that if the content of the last newline doesn't end with > then keep the whitespace?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) Jul 30, 2018

Copy link

Copy Markdown

Collaborator Author

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.

Yes, I close the PR and open new one #7401 with the new regex.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/Microsoft.PowerShell.Commands.Management/singleshell/installer/MshManagementMshSnapin.cs Outdated

		/// MshManagementMshSnapin (or MshManagementMshSnapinInstaller) is a class for facilitating registry
		/// of necessary information for monad management mshsnapin.
		///
		/// This class will be built with monad management dll

Steve Lee (SteveL-MSFT) Jul 27, 2018

Copy link

Copy Markdown

Member

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 should just remove this line, doesn't add value

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) Jul 27, 2018

Copy link

Copy Markdown

Collaborator Author

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.

Removed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/Microsoft.PowerShell.Commands.Utility/singleshell/installer/MshUtilityMshSnapin.cs Outdated

		/// MshUtilityMshSnapin (or MshUtilityMshSnapinInstaller) is a class for facilitating registry
		/// of necessary information for monad utility mshsnapin.
		///
		/// This class will be built with monad utility dll

Steve Lee (SteveL-MSFT) Jul 27, 2018

Copy link

Copy Markdown

Member

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 this line

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHost.cs Outdated

		///
		/// The exit code for the shell.
		///
		/// NTRAID#Windows OS Bugs-1036968-2005/01/20-sburns The behavior here is related to monitor work. The low word of the

Steve Lee (SteveL-MSFT) Jul 27, 2018

Copy link

Copy Markdown

Member

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 "NTRAID#Windows OS Bugs-1036968-2005/01/20-sburns ". NTRAID is a really old bug tracking system that is no longer running so no way to open this bug.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) Jul 27, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Collaborator Author

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.

Removed.
There are many such comments so all comments reviewed and fixed in the file.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHost.cs

		///
		/// NTRAID#Windows OS Bugs-1036968-2005/01/20-sburns The behavior here is related to monitor work. The low word of the
		/// exit code is available for the user. The high word is reserved for the shell and monitor.
		///

Steve Lee (SteveL-MSFT) Jul 27, 2018

Copy link

Copy Markdown

Member

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.

Keep whitespace

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHost.cs

		/// - if ctrl-break is pressed, with 0xFFFE0000
		/// - if the shell.exe is passed a bad command-line parameter, with 0xFFFD0000.
		/// - if the shell.exe crashes, with 0x00000000
		///

Steve Lee (SteveL-MSFT) Jul 27, 2018

Copy link

Copy Markdown

Member

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.

Keep whitespace

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHost.cs

		/// possibly due to an error). If not, the shell process crashed. If the shell.exe exit code is x00000000 (crashed)
		/// or 0xFFFE0000 (user hit ctrl-break), the monitor should restart the shell.exe. Otherwise, the monitor should exit
		/// with the same exit code as the shell.exe.
		///

Steve Lee (SteveL-MSFT) Jul 27, 2018

Copy link

Copy Markdown

Member

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.

Keep whitespace

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/Microsoft.WSMan.Management/WsManHelper.cs

		}

		/// <summary>
		///

Steve Lee (SteveL-MSFT) Jul 27, 2018

Copy link

Copy Markdown

Member

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.

No content in <summary/>

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) Jul 27, 2018

Copy link

Copy Markdown

Collaborator Author

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 have tons empty comments - I can not add useful comments everywhere.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Steve Lee (SteveL-MSFT) Jul 27, 2018

Copy link

Copy Markdown

Member

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 can treat this as a separate issue outside of this PR

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) added 2 commits

July 27, 2018 08:39


          Address SteveL-MSFT's comments

d59fa73


          Merge branch 'master' into remove-empty-xml-comment-lines

5b37edf

Ilya (iSazonov) mentioned this pull request

Jul 30, 2018

Remove empty xml comment lines #7401

Merged

6 of 11 tasks

Ilya (iSazonov) commented Jul 30, 2018

Copy link

Copy Markdown

Collaborator Author

New PR #7401

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Ilya (iSazonov) closed this

Jul 30, 2018

Ilya (iSazonov) deleted the remove-empty-xml-comment-lines branch

July 30, 2018 04:52

Reviewers

Steve Lee (SteveL-MSFT)

SteveL-MSFT requested changes

Travis Plunk (TravisEz13)

Awaiting requested review from TravisEz13

TravisEz13 is a code owner

Dan Travison (dantraMSFT)

Awaiting requested review from dantraMSFT

Aditya Patwardhan (adityapatwardhan)

Awaiting requested review from adityapatwardhan

adityapatwardhan is a code owner

Andrew (anmenaga)

Awaiting requested review from anmenaga

Bruce Payette (BrucePay)

Awaiting requested review from BrucePay

Dongbo Wang (daxian-dbw)

Awaiting requested review from daxian-dbw

daxian-dbw is a code owner

Francisco Gamino (Francisco-Gamino)

Awaiting requested review from Francisco-Gamino

James Truher (JamesWTruher)

Awaiting requested review from JamesWTruher

Mike Richmond (mirichmo)

Awaiting requested review from mirichmo

Paul Higinbotham (PaulHigin)

Awaiting requested review from PaulHigin

Uh oh!

There was an error while loading. Please reload this page.

Assignees

Ilya (iSazonov)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

Ilya (iSazonov) commented Jul 25, 2018

PR Summary

PR Checklist

Uh oh!

Steve Lee (SteveL-MSFT) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) Jul 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) commented Jul 30, 2018

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Ilya (iSazonov) Jul 27, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.