Skip to content
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

Performed Copyediting for Minor Typos #1167

Open
wants to merge 11 commits into
base: draft-v8
Choose a base branch
from

Conversation

danielparvin
Copy link

Fixed various typos and minor formatting issues.

@danielparvin
Copy link
Author

danielparvin commented Sep 2, 2024 via email

Copy link
Contributor

@Nigel-Ecma Nigel-Ecma left a comment

Choose a reason for hiding this comment

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

Well spotted, thanks!
Most of the English grammar ones look good however I will leave @RexJaeschke to check them. The typo fixes all look correct. The stylistic use of en/em-dash and escapes I'll defer to Rex again.
Unfortunately the C# grammar change is wrong and needs to be removed.

standard/classes.md Outdated Show resolved Hide resolved
standard/classes.md Outdated Show resolved Hide resolved
standard/conversions.md Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/classes.md Show resolved Hide resolved
standard/delegates.md Outdated Show resolved Hide resolved
standard/classes.md Outdated Show resolved Hide resolved
@RexJaeschke
Copy link
Contributor

@danielparvin I just added my feedback as comments. I am quite new to the PR-reviewing process, and now that I'm done I just learned I could have turned my prose into specific code edits. C'est la vie!

@RexJaeschke
Copy link
Contributor

I just resolved the one conflict, which was introduced because of the very recent merging of PR #1172 and/or PR #1174.

Copy link
Contributor

@Nigel-Ecma Nigel-Ecma left a comment

Choose a reason for hiding this comment

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

One comment, will defer to @RexJaeschke on that, otherwise LGTM

standard/expressions.md Outdated Show resolved Hide resolved
Copy link
Contributor

@Nigel-Ecma Nigel-Ecma left a comment

Choose a reason for hiding this comment

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

Spotted one new change which needs to be reverted.

Otherwise looks OK to me.

standard/expressions.md Outdated Show resolved Hide resolved
Copy link
Contributor

@Nigel-Ecma Nigel-Ecma left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@Nigel-Ecma Nigel-Ecma left a comment

Choose a reason for hiding this comment

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

LGTM

@danielparvin – All the renumbering looks OK to me but you didn’t need to do this…

The numbers were off due to the introduction of the clause for the null-forgiving expression which pushed all the following clauses up one. There is an automatic renumbering process which fixes issues like this – see CONTRIBUTING.md at the top-level of the repo.

@danielparvin
Copy link
Author

LGTM

@danielparvin – All the renumbering looks OK to me but you didn’t need to do this…

The numbers were off due to the introduction of the clause for the null-forgiving expression which pushed all the following clauses up one. There is an automatic renumbering process which fixes issues like this – see CONTRIBUTING.md at the top-level of the repo.

@Nigel-Ecma , good to know; thank you.

@danielparvin
Copy link
Author

@Nigel-Ecma or @RexJaeschke , are you waiting on any other actions from me? Thanks!

@@ -23,9 +23,9 @@ For expressions which occur as subexpressions of larger expressions, with the no

- A namespace. An expression with this classification can only appear as the left-hand side of a *member_access* ([§12.8.7](expressions.md#1287-member-access)). In any other context, an expression classified as a namespace causes a compile-time error.
- A type. An expression with this classification can only appear as the left-hand side of a *member_access* ([§12.8.7](expressions.md#1287-member-access)). In any other context, an expression classified as a type causes a compile-time error.
- A method group, which is a set of overloaded methods resulting from a member lookup ([§12.5](expressions.md#125-member-lookup)). A method group may have an associated instance expression and an associated type argument list. When an instance method is invoked, the result of evaluating the instance expression becomes the instance represented by `this` ([§12.8.14](expressions.md#12814-this-access)). A method group is permitted in an *invocation_expression* ([§12.8.10](expressions.md#12810-invocation-expressions)) or a *delegate_creation_expression* ([§12.8.17.6](expressions.md#128176-delegate-creation-expressions)), and can be implicitly converted to a compatible delegate type ([§10.8](conversions.md#108-method-group-conversions)). In any other context, an expression classified as a method group causes a compile-time error.
- A method group, which is a set of overloaded methods resulting from a member lookup ([§12.5](expressions.md#125-member-lookup)). A method group may have an associated instance expression and an associated type argument list. When an instance method is invoked, the result of evaluating the instance expression becomes the instance represented by `this` ([§12.8.14](expressions.md#12814-this-access)). A method group is permitted in an *invocation_expression* ([§12.8.10](expressions.md#12810-invocation-expressions)) or a *delegate_creation_expression* ([§12.8.17.6](expressions.md#128166-delegate-creation-expressions)), and can be implicitly converted to a compatible delegate type ([§10.8](conversions.md#108-method-group-conversions)). In any other context, an expression classified as a method group causes a compile-time error.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo

Suggested change
- A method group, which is a set of overloaded methods resulting from a member lookup ([§12.5](expressions.md#125-member-lookup)). A method group may have an associated instance expression and an associated type argument list. When an instance method is invoked, the result of evaluating the instance expression becomes the instance represented by `this` ([§12.8.14](expressions.md#12814-this-access)). A method group is permitted in an *invocation_expression* ([§12.8.10](expressions.md#12810-invocation-expressions)) or a *delegate_creation_expression* ([§12.8.17.6](expressions.md#128166-delegate-creation-expressions)), and can be implicitly converted to a compatible delegate type ([§10.8](conversions.md#108-method-group-conversions)). In any other context, an expression classified as a method group causes a compile-time error.
- A method group, which is a set of overloaded methods resulting from a member lookup ([§12.5](expressions.md#125-member-lookup)). A method group may have an associated instance expression and an associated type argument list. When an instance method is invoked, the result of evaluating the instance expression becomes the instance represented by `this` ([§12.8.14](expressions.md#12814-this-access)). A method group is permitted in an *invocation_expression* ([§12.8.10](expressions.md#12810-invocation-expressions)) or a *delegate_creation_expression* ([§12.8.17.6](expressions.md#128176-delegate-creation-expressions)), and can be implicitly converted to a compatible delegate type ([§10.8](conversions.md#108-method-group-conversions)). In any other context, an expression classified as a method group causes a compile-time error.

Copy link
Contributor

Choose a reason for hiding this comment

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

@danielparvin & @RexJaeschke – I assumed it was just in the queue to merge but on checking I saw the checks had not run and the WordConvertor is barfing on a typo (no idea why the renumbering action couldn’t match the section title – @BillWagner?). I've made the suggestion above which hopefully fixes it. Once that is accepted this should hopefully go into the merge queue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants