Skip to content

adding Try .NET for System.String.* #2971

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

Closed

Conversation

WilliamAntonRohm
Copy link
Contributor

@WilliamAntonRohm WilliamAntonRohm commented Aug 8, 2019

per https://dev.azure.com/mseng/TechnicalContent/_workitems/edit/1558837/

Sample code PR is dotnet/samples#1149

This PR includes the following member groups and members:

  • Compare
  • Contains
  • IndexOf
  • IsNullOrEmpty
  • Join
  • Length
  • Replace
  • Split
  • Substring
  • Trim

@mairaw mairaw added 🚧 Hold for related PR Indicates a PR can only be merged when other related PRs are merged (see comments for links) vendor-project Indicates the issue/pr is related to a vendor project. labels Aug 8, 2019
@mairaw mairaw added this to the August 2019 milestone Aug 8, 2019
@WilliamAntonRohm
Copy link
Contributor Author

Triggering new build

@mairaw
Copy link
Contributor

mairaw commented Aug 9, 2019

dotnet/samples#1149 was merged now. @WilliamAntonRohm please see my merge comments there for some changes you'll have to make.
Now that the samples PR is merged, I've triggered a build of the master branch of dotnet-api-docs to grab the latest samples version. Once that finishes building, we can reopen this PR to grab the latest version as well. Getting the dependency tree updated is a bit complicated.

@mairaw mairaw removed the 🚧 Hold for related PR Indicates a PR can only be merged when other related PRs are merged (see comments for links) label Aug 9, 2019
@mairaw mairaw closed this Aug 9, 2019
@mairaw mairaw reopened this Aug 9, 2019
@WilliamAntonRohm
Copy link
Contributor Author

@mairaw -- Maira, I've removed "-interactive" for the 6 sample files where you undid changes.

Copy link
Contributor

@mairaw mairaw left a comment

Choose a reason for hiding this comment

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

Looks good overall @WilliamAntonRohm, but we need to fix a few things before merging.

@WilliamAntonRohm
Copy link
Contributor Author

@mairaw -- thank you for your review. I'll make these changes shortly.

@WilliamAntonRohm
Copy link
Contributor Author

@mairaw -- I've addressed your review comments in String.xml here and in the sample files PR

@mairaw
Copy link
Contributor

mairaw commented Aug 14, 2019

I kicked off a new build for the master branch which will allow me to pull the latest samples into this PR. I'll close and reopen this PR when that happens.

@mairaw mairaw closed this Aug 15, 2019
@mairaw mairaw reopened this Aug 15, 2019
@mairaw
Copy link
Contributor

mairaw commented Aug 15, 2019

Starting new build...

@mairaw
Copy link
Contributor

mairaw commented Aug 15, 2019

Build has finished. Have you tested all your changes already @WilliamAntonRohm?
I'll start reviewing them as well.

@WilliamAntonRohm
Copy link
Contributor Author

@mairaw -- thank you for your review. I am double-checking all the samples under these 10 member groups/members.

@mairaw
Copy link
Contributor

mairaw commented Aug 15, 2019

Sounds good. I'm going through them as well to double check.

Copy link
Contributor

@mairaw mairaw left a comment

Choose a reason for hiding this comment

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

Finished my new review. Please take a look.

@mairaw
Copy link
Contributor

mairaw commented Aug 21, 2019

@WilliamAntonRohm any updates here?

@WilliamAntonRohm
Copy link
Contributor Author

WilliamAntonRohm commented Aug 21, 2019

@mairaw -- I have been through Compare and updated the samples to match, adding three new interactive samples (comp3.cs [twice] and comp4.cs); see my note above regarding remarks.cs.

Contains is good to go.

IndexOf has been checked & updated.

@mairaw
Copy link
Contributor

mairaw commented Aug 22, 2019

@WilliamAntonRohm I don't see any updates from today. Also, look at my comments for dotnet/samples#1192 and see if you need to make any changes based on the changes I've made on top of that. Thanks!

@WilliamAntonRohm
Copy link
Contributor Author

@mairaw -- I am hoping this build will succeed by morning.

Thank you for your close review of the latest samples, and for showing me their preferred formatting -- I'm checking the other post-interactivated samples.

@mairaw
Copy link
Contributor

mairaw commented Aug 23, 2019

I didn't include that as part of the task description, so I'm just doing as I review the work.

@WilliamAntonRohm
Copy link
Contributor Author

@mairaw -- Maira, I believe this new String.xml is ready to go live. Thank you for your patience with my learning curve -- I anticipate the next ones (regex) will be much smoother.

@WilliamAntonRohm
Copy link
Contributor Author

@mairaw -- I followed your master vs. upstream recommendations and just now created a replacement (new #3070) for this PR 2971, with only the one file changed (String.xml itself). After your review, I'll make any changes in that new branch.

@mairaw
Copy link
Contributor

mairaw commented Oct 23, 2019

We have new merge conflicts 😢

@WilliamAntonRohm
Copy link
Contributor Author

WilliamAntonRohm commented Oct 23, 2019

@mairaw -- dang! I'm unable to see the conflicts in this PR or its replacement #3070, as the "Resolve conflicts" button is grayed-out, presumably a byproduct of my permissions level; I am likewise unable to assign an owner or reviewer. If you could send me a raw text file of the conflicts I can re-create my additions.

@mairaw
Copy link
Contributor

mairaw commented Oct 24, 2019

I can't see them either:
image

You need to use the command line to resolve this. If you need help, please let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Runtime vendor-project Indicates the issue/pr is related to a vendor project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants