GeezerVison

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

GeezerVison is showing github.com — Clean up MshObject.cs and MshMemberInfo.cs by daxian-dbw · Pull Request #7446 · PowerShell/PowerShell

Conversation

Dongbo Wang (daxian-dbw) commented Aug 3, 2018 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

PR Summary

Separated from #7413
Clean up MshObject.cs and MshMemberInfo.cs.

/cc Ilya (@iSazonov) Staffan Gustafsson (@powercode)

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

Dongbo Wang (daxian-dbw) requested a review from Bruce Payette (BrucePay) as a code owner

August 3, 2018 19:26

Dongbo Wang (daxian-dbw) requested review from Travis Plunk (TravisEz13), Aditya Patwardhan (adityapatwardhan), Andrew (anmenaga), Ilya (iSazonov) and Staffan Gustafsson (powercode) and removed request for Bruce Payette (BrucePay)

August 3, 2018 19:26

Dongbo Wang (daxian-dbw) assigned Ilya (iSazonov)

Aug 3, 2018

Staffan Gustafsson (powercode) commented Aug 3, 2018

Copy link

Copy Markdown

Collaborator

Looks great! Ship it!

daxian-dbw and powercode reacted with laugh emoji

All reactions

😄 2 reactions

Sorry, something went wrong.

Uh oh!

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

Dongbo Wang (daxian-dbw) force-pushed the cleanup branch from 2cd7fb3 to 402b90a Compare

August 3, 2018 19:28

Dongbo Wang (daxian-dbw) mentioned this pull request

Aug 3, 2018

Improving performance of Import-CSV #7413

Merged

6 of 10 tasks

Steve Lee (SteveL-MSFT) approved these changes

Aug 3, 2018

View reviewed changes

Ilya (iSazonov) reviewed

Aug 5, 2018

View reviewed changes

Ilya (iSazonov) left a comment

Copy link

Copy Markdown

Collaborator

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 marked not all issues (one per type).

I absolutely don't agree use new pattern ( => ) in one file. If we want use this we should fix all code base.
In typical PR we modify some files and if the files have different style this is a difficulty for contributor.
I would like us to use the same styles and formatting throughout the code. These will greatly improve code manageability.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

src/System.Management.Automation/engine/MshObject.cs Outdated

Ilya (iSazonov) Aug 5, 2018

Copy link

Copy Markdown

Collaborator

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.

Did we say about out TypeTabel typeTable?

Sorry, something went wrong.

Uh oh!

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

All reactions

Staffan Gustafsson (powercode) Sep 4, 2018

Copy link

Copy Markdown

Collaborator

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.

That it was less noisy and repetitive?

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

src/System.Management.Automation/engine/MshObject.cs Outdated

Ilya (iSazonov) Aug 5, 2018

Copy link

Copy Markdown

Collaborator

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.

How many delegate () {} patterns we have in our code? I unlike use new pattern in one place and add a chaos. I'd prefer use the same pattern in all code and if we want new pattern I'd expect that we fix all code in one PR.

Sorry, something went wrong.

Uh oh!

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

All reactions

Staffan Gustafsson (powercode) Sep 4, 2018

Copy link

Copy Markdown

Collaborator

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 think we should write new code using new patterns. If someone wants to clean up older code, fine. But I would really dislike having to use some old construct just because some other old code hasn't been updated.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

src/System.Management.Automation/engine/MshObject.cs Outdated

Ilya (iSazonov) Aug 5, 2018

Copy link

Copy Markdown

Collaborator

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.

Why last arguments is on one line? And why we don't use named arguments for constants?

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

src/System.Management.Automation/engine/MshObject.cs Outdated

Ilya (iSazonov) Aug 5, 2018

Copy link

Copy Markdown

Collaborator

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 unlike use new pattern in one file and add a chaos. I'd prefer use the same pattern in all code and if we want new pattern I'd expect that we fix all code.

Sorry, something went wrong.

Uh oh!

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

All reactions

Staffan Gustafsson (powercode) Sep 4, 2018

Copy link

Copy Markdown

Collaborator

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 like using new patterns in new code. I don't in any way feel that it in any way creates chaos. We must have completely different ways of using that word.

If you get to a file using the older pattern, you recognize it as older code, but we are capable of reading and understanding both versions.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

src/System.Management.Automation/engine/MshObject.cs Outdated

Ilya (iSazonov) Aug 5, 2018

Copy link

Copy Markdown

Collaborator

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.

Extra space after (XmlNode).

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

src/System.Management.Automation/engine/MshMemberInfo.cs Outdated

Ilya (iSazonov) Aug 5, 2018

Copy link

Copy Markdown

Collaborator

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.

Should we use spaces? { this.instance }

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

src/System.Management.Automation/engine/MshMemberInfo.cs Outdated

Ilya (iSazonov) Aug 5, 2018

Copy link

Copy Markdown

Collaborator

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

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

src/System.Management.Automation/engine/MshMemberInfo.cs Outdated

Ilya (iSazonov) Aug 5, 2018

Copy link

Copy Markdown

Collaborator

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.

Why this is on new line?

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

src/System.Management.Automation/engine/MshMemberInfo.cs Outdated

Ilya (iSazonov) Aug 5, 2018

Copy link

Copy Markdown

Collaborator

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.

Extra space.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

src/System.Management.Automation/engine/MshMemberInfo.cs Outdated

Ilya (iSazonov) Aug 5, 2018 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Collaborator

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.

Extra space.

Sorry, something went wrong.

Uh oh!

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

All reactions

stale Bot commented Sep 4, 2018

Copy link

Copy Markdown

This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days.
Thank you for your contributions.
Community members are welcome to grab these works.

All reactions

Sorry, something went wrong.

Uh oh!

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

stale Bot added Stale and removed Stale labels

Sep 4, 2018

Staffan Gustafsson (powercode) approved these changes

Sep 4, 2018

View reviewed changes

Comment thread

src/System.Management.Automation/engine/MshObject.cs Outdated

Staffan Gustafsson (powercode) Sep 4, 2018

Copy link

Copy Markdown

Collaborator

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.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

src/System.Management.Automation/engine/MshObject.cs Outdated

Staffan Gustafsson (powercode) Sep 4, 2018

Copy link

Copy Markdown

Collaborator

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 like using new patterns in new code. I don't in any way feel that it in any way creates chaos. We must have completely different ways of using that word.

If you get to a file using the older pattern, you recognize it as older code, but we are capable of reading and understanding both versions.

Sorry, something went wrong.

Uh oh!

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

All reactions

Travis Plunk (TravisEz13) commented Sep 12, 2018

Copy link

Copy Markdown

Member

Dongbo Wang (@daxian-dbw) Can you resolve conflicts?

All reactions

Sorry, something went wrong.

Uh oh!

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

Dongbo Wang (daxian-dbw) force-pushed the cleanup branch from 402b90a to dd79054 Compare

September 12, 2018 18:14

Dongbo Wang (daxian-dbw) commented Sep 12, 2018

Copy link

Copy Markdown

Member Author

The PR has been rebased. Ilya (@iSazonov) maintainers can push changes to PRs, so please feel free to address the issues that are brought up in your comments.

All reactions

Sorry, something went wrong.

Uh oh!

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

Ilya (iSazonov) commented Sep 14, 2018

Copy link

Copy Markdown

Collaborator

Steve Lee (@SteveL-MSFT) Dongbo Wang (@daxian-dbw) New commits was added.

All reactions

Sorry, something went wrong.

Uh oh!

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

Steve Lee (SteveL-MSFT) commented Sep 24, 2018

Copy link

Copy Markdown

Member

Looks like we just need to resolve the merge conflict

All reactions

Sorry, something went wrong.

Uh oh!

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

Travis Plunk (TravisEz13) commented Sep 24, 2018

Copy link

Copy Markdown

Member

The code needs to be rebased to fix the test issue on linux as well

All reactions

Sorry, something went wrong.

Uh oh!

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

Gustafsson, Staffan and others added 4 commits

September 25, 2018 11:32


          Cleanup of MshObject.cs

0e7859b


          [Feature] Cleanup of MshMemberInfo.cs

c9c892e


          Style fixes

be0ae0b


          Replace String.<> with string.<>

Ilya (iSazonov) force-pushed the cleanup branch from 27394ee to 8061354 Compare

September 25, 2018 06:47

Ilya (iSazonov) commented Sep 25, 2018

Copy link

Copy Markdown

Collaborator

Rebased.

All reactions

Sorry, something went wrong.

Uh oh!

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


          Fix typo

02efdc2

Ilya (iSazonov) commented Sep 25, 2018

Copy link

Copy Markdown

Collaborator

CI Appveyor temporary failed.

All reactions

Sorry, something went wrong.

Uh oh!

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

Steve Lee (SteveL-MSFT) approved these changes

Sep 25, 2018

View reviewed changes

Travis Plunk (TravisEz13) approved these changes

Sep 26, 2018

View reviewed changes

Ilya (iSazonov) merged commit 0cce2ae into PowerShell:master

Sep 26, 2018

Dongbo Wang (daxian-dbw) deleted the cleanup branch

October 12, 2018 22:40

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request

Aug 6, 2026


          Clean up MshObject.cs and MshMemberInfo.cs (PowerShell#7446)

b13baed

Clean up style issues.

Reviewers

Ilya (iSazonov)

iSazonov left review comments

Travis Plunk (TravisEz13)

TravisEz13 approved these changes

Steve Lee (SteveL-MSFT)

SteveL-MSFT approved these changes

Andrew (anmenaga)

Awaiting requested review from anmenaga

Aditya Patwardhan (adityapatwardhan)

Awaiting requested review from adityapatwardhan

+1 more reviewer

Staffan Gustafsson (powercode)

powercode approved these changes

Reviewers whose approvals may not affect merge requirements

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.

5 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

Dongbo Wang (daxian-dbw) commented Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

Staffan Gustafsson (powercode) commented Aug 3, 2018

Uh oh!

Ilya (iSazonov) 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!

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) Aug 5, 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!

stale Bot commented Sep 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Travis Plunk (TravisEz13) commented Sep 12, 2018

Uh oh!

Dongbo Wang (daxian-dbw) commented Sep 12, 2018

Uh oh!

Ilya (iSazonov) commented Sep 14, 2018

Uh oh!

Steve Lee (SteveL-MSFT) commented Sep 24, 2018

Uh oh!

Travis Plunk (TravisEz13) commented Sep 24, 2018

Uh oh!

Ilya (iSazonov) commented Sep 25, 2018

Uh oh!

Ilya (iSazonov) commented Sep 25, 2018

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Dongbo Wang (daxian-dbw) commented Aug 3, 2018 •

edited

Loading

Uh oh!

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

Ilya (iSazonov) Aug 5, 2018 •

edited

Loading

Uh oh!

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