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

C# 6.0 Feature: using static #9

Merged
merged 10 commits into from
Jul 31, 2021
Merged

C# 6.0 Feature: using static #9

merged 10 commits into from
Jul 31, 2021

Conversation

BillWagner
Copy link
Member

@BillWagner BillWagner commented Oct 23, 2020

Current spec text for using static.

Creating a draft PR because of outstanding TODO item. TODO item is complete.

Fixes #51
Fixes #199

@BillWagner BillWagner force-pushed the feature-using-static branch 2 times, most recently from 63a4c98 to 80bb520 Compare October 26, 2020 20:45
@BillWagner BillWagner mentioned this pull request Nov 30, 2020
@RexJaeschke RexJaeschke added this to the C# 6 milestone Mar 17, 2021
@BillWagner BillWagner force-pushed the feature-using-static branch from 80bb520 to 6e691c6 Compare March 17, 2021 15:04
@BillWagner BillWagner requested a review from RexJaeschke July 7, 2021 19:17
@BillWagner BillWagner marked this pull request as ready for review July 7, 2021 19:17
Current spec text for using static

proofread fixes
I chose to use non-descript names rather than provide an example from the class library. I think that's sufficient to understand, and doesn't beg more questions about the design decision (even in an informative example).
@BillWagner BillWagner force-pushed the feature-using-static branch from c5ce449 to 2066313 Compare July 7, 2021 19:24
@BillWagner BillWagner requested a review from jskeet July 7, 2021 19:39
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.

Broadly fine - just some formatting nits.

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

@RexJaeschke RexJaeschke left a comment

Choose a reason for hiding this comment

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

I have nothing more to add.

BillWagner and others added 3 commits July 19, 2021 16:45
Co-authored-by: Jon Skeet <skeet@pobox.com>
I wanted to check other clauses in the standard to compare. Jon is correct.
standard/namespaces.md Outdated Show resolved Hide resolved
standard/namespaces.md Outdated Show resolved Hide resolved
@jskeet
Copy link
Contributor

jskeet commented Jul 28, 2021

Meeting is happy to merge when formatting is sorted.

Make the following changes to the code samples in this clause:

- Add a blank line between peer elements *except* for multiple `using` directives and multiple `alias` directives. (I did add a blank line between `using` and `alias` directives.
- Use Allman brace style on non-empty `class` declarations. (I'd prefer using Allman brace style on methods, but the standard currently uses K&R style for methods consistently.
Copy link
Member Author

@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.

@jskeet I tried to make the changes in this clause to develop our general practice.

I followed the notes from the meeting, and then applied my own opinion on whitespace, because I was doing the work. 😀

@@ -669,7 +713,9 @@ Using this notation, the meaning of a *qualified_alias_member* is determined as
> *Note*: The identifier `global` has special meaning only when used as the left-hand identifier of a *qualified_alias_name*. It is not a keyword and it is not itself an alias; it is a contextual keyword ([§7.4.4](lexical-structure.md#744-keywords)). In the code:
> ```csharp
> class A { }
> class C {
>
> class C
Copy link
Member Author

Choose a reason for hiding this comment

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

This was one of 2 instances for K&R braces on class declarations. That's why I changed this one.

> class C {
>
> class C
> {
Copy link
Member Author

Choose a reason for hiding this comment

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

second instance of K&R braces on a class

@@ -166,6 +167,7 @@ Within global attributes and member declarations in a compilation unit or namesp
> namespace N3
> {
> using A = N1.N2.A;
>
Copy link
Member Author

Choose a reason for hiding this comment

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

@jskeet This was one type of change I wanted to check on. I think it's better by having the blank line, but it's a very mild preferences.

For our "coding style", I'd like to say different elements should have a blank line between them. On the other hand, I like a rule that doesn't add a blank line where the element is an empty class or namespace.

See also line 179, 192, 201-203.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mmm, interesting. I probably wouldn't have added blank lines in 179, 192, 201-203... but I'm quite happy for us to do so, and that should probably be the default. We should just allow ourselves to have an exception for things like:

public void M(int x) {}
public void M(string y) {}
public void M(int x, string y = null) {}

... in an example about overload resolution, for example.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. I think that's easily handled by adding methods to my exception in the previous comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

I opened #358 to start the coding standard discussion.

I'll merge this.

@BillWagner BillWagner merged commit cea1284 into draft-v6 Jul 31, 2021
@BillWagner BillWagner deleted the feature-using-static branch July 31, 2021 20:40
BillWagner added a commit that referenced this pull request Aug 31, 2021
PR #9 has been merged, so this is done now.
BillWagner added a commit that referenced this pull request Aug 31, 2021
PR #9 has been merged, so this is done now.
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.

14.5.4, "Using static directives": Example needed Using static directive
4 participants