Clean up MshObject.cs and MshMemberInfo.cs #7446
Conversation
daxian-dbw
requested review from
TravisEz13,
adityapatwardhan,
anmenaga,
iSazonov and
powercode
and removed request for
BrucePay
|
Looks great! Ship it! |
iSazonov
left a comment
iSazonov
left a comment
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Did we say about out TypeTabel typeTable?
There was a problem hiding this comment.
That it was less noisy and repetitive?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Why last arguments is on one line? And why we don't use named arguments for constants?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Extra space after (XmlNode).
There was a problem hiding this comment.
Should we use spaces? { this.instance }
|
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@daxian-dbw Can you resolve conflicts? |
|
The PR has been rebased. @iSazonov maintainers can push changes to PRs, so please feel free to address the issues that are brought up in your comments. |
|
@SteveL-MSFT @daxian-dbw New commits was added. |
|
Looks like we just need to resolve the merge conflict |
|
The code needs to be rebased to fix the test issue on linux as well |
|
Rebased. |
|
CI Appveyor temporary failed. |
5 participants
PR Summary
Separated from #7413
Clean up
MshObject.csandMshMemberInfo.cs./cc @iSazonov @powercode
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