-
Notifications
You must be signed in to change notification settings - Fork 6k
23). Add topic files 63 to 66 #17892
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
23). Add topic files 63 to 66 #17892
Conversation
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.
Great work @jameshkramer
I had a couple comments to consider, along with a question for Maira
Billing, | ||
} | ||
|
||
private AddressUse addressType; |
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 realize this is expanding scope, but is this a good time to remove the private
fields and change the later properties to auto-implemented properties?
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 don't think this is my call. It certainly is beyond scope, because it is well beyond the attitude of "get the articles merged and go from there". That does necessarily argue against it - we have already gone beyond scope, I think, which is not necessarily bad.
I might or might not be the right person to make such changes. I don't really understand what you propose, but if it only requires knowledge of C# I would be fine. If it depends on knowing LINQ, I might not be the best person.
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.
Let's keep the task focused on bringing the articles over. And then you can make the improvements later @BillWagner?
@jameshkramer please resolve the new merge conflicts |
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.
LGTM. Left a few comments to be addressed before merging.
Co-Authored-By: Maira Wenzel <[email protected]>
Co-Authored-By: Maira Wenzel <[email protected]>
Co-Authored-By: Maira Wenzel <[email protected]>
Co-Authored-By: Maira Wenzel <[email protected]>
Co-Authored-By: Maira Wenzel <[email protected]>
Co-Authored-By: Maira Wenzel <[email protected]>
Co-Authored-By: Maira Wenzel <[email protected]>
Co-Authored-By: Maira Wenzel <[email protected]>
This PR is for user story #1576037.
It adds four article files, makes corresponding deletions from the C# and VB folders, and adds redirection entries. It also fixes a few article files from previous commits.
Please merge #17846 before merging this.
cc: @tfosmark @marteeleigh
Contributes to #4728