update interactive samples in string.format #2553
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
contributes to dotnet#2539 Relies on dotnet/samples#953 When possible, update the snippets to work as included snippets. Otherwise, turn off the interactive samples on those samples.
BillWagner
added
the
🚧 Hold for related PR
|
closing and reopening to start a new build |
BillWagner
removed
the
🚧 Hold for related PR
BillWagner
added a commit
to BillWagner/samples
that referenced
this pull request
Jun 6, 2019
A few samples weren't caught in the first PR. contributes to dotnet/dotnet-api-docs#2553
|
The latest build fixes are in dotnet/samples#957 |
BillWagner
added
the
🚧 Hold for related PR
BillWagner
added a commit
to dotnet/samples
that referenced
this pull request
Jun 6, 2019
A few samples weren't caught in the first PR. contributes to dotnet/dotnet-api-docs#2553
BillWagner
removed
the
🚧 Hold for related PR
|
closing and reopening for a fresh build |
rpetrusha
approved these changes
Jun 6, 2019
There was a problem hiding this comment.
This generally looks good, @BillWagner. Some comments:
- The invariant culture is still used instead of the user's culture. I think I would consider making some of the examples that depend on the user's culture non-interactive. For example, the currency formatting example uses the currency symbol for the invariant culture, which is likely to be mystifying:
There are two examples that depend on culture-specific currency symbols. Potentially, this applies to some of the date/time output examples as well, though using the invariant culture for them is less jarring. - The example on line 5270 consistently times out.
|
Thanks @rpetrusha I fixed the sample on line 5270. (One closing bracket should have been included that was not.) I disabled interactive on the currency examples. I left it on for the datetime ones though. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
contributes to #2539
Relies on dotnet/samples#953
When possible, update the snippets to work as included snippets.
Otherwise, turn off the interactive samples on those samples.
internal review link