-
Notifications
You must be signed in to change notification settings - Fork 743
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
Typo fixes #1960
Typo fixes #1960
Conversation
@@ -374,7 +374,7 @@ private static void IssueWarning(string message) | |||
result.RecordAssertion(AssertionStatus.Warning, message, GetStackTrace()); | |||
} | |||
|
|||
// System.Envionment.StackTrace puts extra entries on top of the stack, at least in some environments | |||
// System.Environment.StackTrace puts extra entries on top of the stack, at least in some environments |
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'd really like to use <see cref="System.Environment.StackTrace"/>
here but that should probably be its own PR and take care of more than just the typo spots.
@@ -52,7 +52,7 @@ public static class FrameworkPackageSettings | |||
|
|||
/// <summary> | |||
/// Full path of the directory to be used for work and result files. | |||
/// This path is provided to tests by the frameowrk TestContext. | |||
/// This path is provided to tests by the framework TestContext. |
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.
Same here with <see cref="TestContext"/>
. Gives really nice IntelliSense coloring and Object Browser linking.
These all look good to merge but I think it should wait till after the release and merging back of the release branch. |
@@ -33,7 +33,7 @@ public enum SpecialValue | |||
{ | |||
/// <summary> | |||
/// Null represents a null value, which cannot be used as an | |||
/// argument to an attriute under .NET 1.x | |||
/// argument to an attribute under .NET 1.x |
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.
Under .NET 1.X 😱 @CharliePoole - Do you think we can get rid of this now, or does it serve other uses?
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.
My guess is that we can probably get rid of SpecialValue entirely, but since this PR was about typos I didn't want to confuse the issue.
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've made #1963 😄
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.
By the way, @jnm2 - I forgot to mention how happy this PR makes my inner-perfectionist. 😄 Nice one!
Looks good to merge for me as well. @rprouse Do you want us to keep merging to master or will that confuse the release process at this point? |
I'm good to merge this. I am going to do another merge from master into the release branch so may as well get this in. |
I spotted a couple and ran ReSpeller over the solution. For a codebase this big, there are hardly any typos!
The XML doc comments and exception message typos are the most important, but if someone is willing to put an eye on all these changes that would be great.