Skip to content

Grammar – Consistency & Cleanup 2 #1291

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

Merged
merged 1 commit into from
Mar 19, 2025

Conversation

Nigel-Ecma
Copy link
Contributor

@Nigel-Ecma Nigel-Ecma commented Mar 17, 2025

[Continues on from #1290 Grammar – Consistency & Cleanup]

This PR removes type_parameter_constraints_clauses orphaned during the v5 -> v6 transition.

In v5 this rule was present in a number of places, e.g.:

class-declaration:
   attributesopt class-modifiersopt partialopt class identifier type-parameter-listopt
   class-baseopt type-parameter-constraints-clausesopt class-body ;opt

Where ever it occurred it was optional. The rule itself was defined as:

type-parameter-constraints-clauses:
   type-parameter-constraints-clause
   type-parameter-constraints-clauses type-parameter-constraints-clause

which is one or more type-parameter-constraints-clause specified using left-recursion.

Combining “optional” with “one or more” produces “zero or more” – which the ANTLR notation provides directly using *, so in v6 the clauses using type-parameter-constraints-clauses all changed to using type-parameter-constraints-clause zero or more times, e.g.:

class_declaration
    : attributes? class_modifier* 'partial'? 'class' identifier
        type_parameter_list? class_base? type_parameter_constraints_clause*
        class_body ';'?
    ;

along with all in-text references to type_parameter_constraints_clauses being changed to type_parameter_constraints_clauses (trailing 's' not in italics).

Job done…

Well not quite type_parameter_constraints_clauses was not deleted but orphaned, and a few of those in-text references were missed and/or the English grammar around them not fixed up.

This PR simply completes the job.

(And there is a little trailing whitespace cleanup done as my check-in “lint” action barfs at them.)

@Nigel-Ecma Nigel-Ecma added this to the C# 8.0 milestone Mar 17, 2025
@Nigel-Ecma Nigel-Ecma self-assigned this Mar 17, 2025
@Nigel-Ecma Nigel-Ecma marked this pull request as ready for review March 17, 2025 03:56
@Nigel-Ecma Nigel-Ecma added the meeting: discuss This issue should be discussed at the next TC49-TG2 meeting label Mar 18, 2025
@jskeet
Copy link
Contributor

jskeet commented Mar 18, 2025

Again we've got removal of trailing whitespace... @BillWagner is that something we can lint for? It would be good to remove it in a single PR and then make sure it doesn't creep in again. (I'm fine to keep the changes in this PR for simplicity.)

Copy link
Contributor

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

Again, I'm pretty confident I understand this one...

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.

This also LGTM. Let's :shipit: at our next meeting.

@BillWagner
Copy link
Member

Again we've got removal of trailing whitespace... @BillWagner is that something we can lint for? It would be good to remove it in a single PR and then make sure it doesn't creep in again. (I'm fine to keep the changes in this PR for simplicity.)

TL;DR; Instead of doing all the work to automate whitespace removal, I suggest that if we create a PR with a lot of whitespace cleanup, that PR author should recommend reviewers "hide whitespace".

Here's why: Markdown lint will flag a single trailing whitespace character. However, 2 whitespace characters creates a line break. I agree with the cited author that it's hard to see, so use a blank line instead. So, removing all whitespace likely changes the rendered output.

Instead, let's just keep updating as the tools point out concerns.

@Nigel-Ecma
Copy link
Contributor Author

Here's why: Markdown lint will flag a single trailing whitespace character. However, 2 whitespace characters creates a line break. I agree with the cited author that it's hard to see, so use a blank line instead. So, removing all whitespace likely changes the rendered output.

As many of you know I find Markdown one of the most beautiful languages ever developed and now I learn the face of it been scared by this carbuncle?!? (/s, just in case)

Across the Standard’s md files there are currently 68 occurences of two or more spaces at the end of lines (the Google flavoured markdown trigger for a hard break). A brief glance at just a few showed they had no effect due to other formatting, but an exhaustive check would be required to see if they all could be clobbered.

If a hard break is required a line can be ended with a backslash, which at least is visible. I suggest we make that a Standard requirement and then look at clobbering all current trailing whitespace and flagging it after that (https://github.com/updownpress/markdown-lint/blob/master/rules/009-no-trailing-spaces.md)

@jskeet jskeet merged commit 8306316 into dotnet:draft-v8 Mar 19, 2025
6 checks passed
@Nigel-Ecma Nigel-Ecma deleted the deep_to_wide.2 branch March 19, 2025 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meeting: discuss This issue should be discussed at the next TC49-TG2 meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants