Skip to content

Conversation

@hDoubleE
Copy link
Contributor

@hDoubleE hDoubleE commented Nov 14, 2018

Supported by dotnet/samples#462
Modified the Format Strings section of C# Strings using @pkulikov suggestions from #9002 . This is a learning curve for me so I encourage feedback and modifications to writing as needed.

Samples: Should these be in the same PR or a different one? I have a sample file for interpolation I can include if needed. Also, should the code snips for the two methods output the same for contrast or just keep System.Format sample as is. I didn't want to proceed with the samples without clarification.

#9002 (a start)

@hDoubleE hDoubleE requested a review from BillWagner as a code owner November 14, 2018 12:06
@dnfclas
Copy link

dnfclas commented Nov 14, 2018

CLA assistant check
All CLA requirements met.

@BillWagner BillWagner added the ✨ 1st-time docs contributor! Indicates PRs from new contributors to the docs repository label Nov 14, 2018
@BillWagner BillWagner changed the title C# Strings - Format Strings Block [WIP] C# Strings - Format Strings Block Nov 14, 2018
@BillWagner
Copy link
Member

This is a great start @hDoubleE I looked at the markdown changes, and added a few small comments. I also updated the title to add the work-in-progress moniker. Once the samples are done, you can remove that. In the meantime, it prevents us from accidentally merging while you're still working.

This section of the contributor guide has basics on adding samples. For this article, the new snippets should go in samples/snippets/csharp/programming-guide/strings (the 'strings' component is a new folder suggestion)

Should these be in the same PR or a different one?

Because the dotnet/samples is a different repo, it will be 2 PRs. Add links between them so we merge them together.

I have a sample file for interpolation I can include if needed. Also, should the code snips for the two methods output the same for contrast or just keep System.Format sample as is. I didn't want to proceed with the samples without clarification.

That's great. I like the idea of using the same output, but it's not a strong preference.

@hDoubleE
Copy link
Contributor Author

Thanks for the help and feedback @BillWagner and @pkulikov. I'm going to work through the revisions you've suggested and create a PR for the samples. I'm moonlighting this and am a first timer, so bear with me. I thought if I'm going to try and learn the process it might as well be on a change with some substance. Thanks for the support and the style tips. I'll do my best to keep the work moving.

@hDoubleE
Copy link
Contributor Author

I went through and made each suggested change. Thanks for all the great input. Please review for accuracy while I work up the samples. Is it preferable to recycle samples from other pages or generate new material? I'm leaning toward making my own, but maybe it's better to stick with whats available?

@hDoubleE
Copy link
Contributor Author

Hopefully that fixes the relative link paths. I also changed an H3 from "Format Method" to "Composite Formatting" after talking to @pkulikov, it seemed more technically accurate. Agree? I'm working on getting the snippets PR up so we can close this one out. Thanks for all the help!

@hDoubleE
Copy link
Contributor Author

hDoubleE commented Nov 17, 2018

Still getting warnings in Build: Cannot resolve '~/samples/snippets/csharp/programming-guide/strings/Strings_1.cs' relative to 'docs/csharp/programming-guide/strings/index.md'.

The 'strings' folder I made where Strings_1.cs resides in my local fork of dotnet/samples does not exist in samples repo file structure. Please advise what action to take to clear this up.

@pkulikov
Copy link
Contributor

Please advise what action to take to clear this up.

Wait. This PR is built against the master branch of the samples repo. So, we need to wait until the supporting dotnet/samples#462 PR is merged. After it's merged, this PR would be closed/reopened to trigger a new build to check if new snippets are picked up.

Usually, after addressing all the feedback, contributors @-mention the reviewer in the comment to let them know that changes were addressed and PR is ready for the next round of review. That almost always makes the process faster.

@hDoubleE hDoubleE changed the title [WIP] C# Strings - Format Strings Block C# Strings - Format Strings Block Nov 20, 2018
@hDoubleE hDoubleE changed the title C# Strings - Format Strings Block [WIP] C# Strings - Format Strings Block Nov 20, 2018
@hDoubleE
Copy link
Contributor Author

Saw change to review status, then back. What else needs to be done? Any way to include this change?

@BillWagner
Copy link
Member

Thanks for the ping @hDoubleE

I'll review it today, and if all's well, I'll :shipit:

@BillWagner BillWagner changed the title [WIP] C# Strings - Format Strings Block C# Strings - Format Strings Block Nov 20, 2018
@BillWagner
Copy link
Member

closing and re-opening to force a build (and check the snippet includes)

@BillWagner BillWagner closed this Nov 20, 2018
@BillWagner BillWagner reopened this Nov 20, 2018
@BillWagner BillWagner closed this Nov 20, 2018
@BillWagner BillWagner reopened this Nov 20, 2018
Copy link
Member

@BillWagner BillWagner left a comment

Choose a reason for hiding this comment

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

Thanks for all the work, and all the updates on this @hDoubleE It's an impressive amount of work for your first PR with us. We are grateful to have community members like you working with us. 🌠

Also, thanks to @pkulikov who answered questions and provided guidance.

I've reviewed all the changes, and I'll :shipit: now.

Thanks again for contributing to docs.

@BillWagner BillWagner merged commit b911e0d into dotnet:master Nov 20, 2018
@hDoubleE hDoubleE deleted the format-strings branch November 20, 2018 22:51
@hDoubleE
Copy link
Contributor Author

Removed branch:

Correction: first PR ever!!! The learning curve was steep. But nothing like diving in to get started! Thanks @pkulikov for all your help and support and feedback throughout this process. I appreciate the chance to contribute!

One final thing for the group: What can I do better? What mistakes here (many) should I look to avoid in the future?

Thanks again. You guys rock.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

✨ 1st-time docs contributor! Indicates PRs from new contributors to the docs repository

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants