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

use cleaner syntax for C# Generated.cs files #31923

Merged
merged 17 commits into from Mar 19, 2019
Merged

Conversation

Meir017
Copy link
Contributor

@Meir017 Meir017 commented Dec 18, 2018

summary of changes:

  • use lambda-expression for one-line methods

@Meir017 Meir017 requested review from a team as code owners December 18, 2018 23:34
@Meir017 Meir017 changed the title [WIP] use cleaner syntax for C# Generated.cs files use cleaner syntax for C# Generated.cs files Dec 19, 2018
@Meir017
Copy link
Contributor Author

Meir017 commented Dec 20, 2018

@jcouv continuation of #31914 #Closed

@jcouv jcouv added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Dec 20, 2018
@jcouv
Copy link
Member

jcouv commented Dec 20, 2018

using Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax;

I think we have a guideline to have all usings outside the namespace.

If there is a conflict between the usings, then it may be necessary to split the file. Let's hold off on that until things are reviewed in current state though. #Closed


Refers to: src/Compilers/CSharp/Test/Syntax/Generated/Syntax.Test.xml.Generated.cs:8 in 0dfbd1e. [](commit_id = 0dfbd1e, deletion_comment = False)

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM with open question about usings inside namespaces. Thanks

@jcouv jcouv self-assigned this Dec 20, 2018
@Meir017
Copy link
Contributor Author

Meir017 commented Jan 8, 2019

@jcouv any update on this? #Closed

@jaredpar
Copy link
Member

jaredpar commented Jan 8, 2019

@Meir017

I'm not sure about @jcouv but I was waiting for a response to this piece of feedback:

I think we have a guideline to have all usings outside the namespace.

The preference in our repository is using outside the namespace declaration. This PR changes that. #Closed

@Meir017
Copy link
Contributor Author

Meir017 commented Jan 8, 2019

So I should generate two files? One for the internal syntax and one for the public #Closed

@jcouv
Copy link
Member

jcouv commented Jan 8, 2019

So I should generate two files?

You can either revert to use fully-qualified names, or split the file. Thanks #Closed

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Jan 8, 2019

So I should generate two files? One for the internal syntax and one for the public

Why can't you just place the usings outside the namespace? #Closed

@jcouv
Copy link
Member

jcouv commented Jan 8, 2019

Why can't you just place the usings outside the namespace?

I expect this results in ambiguities between public and internal syntax nodes. But if there is a way to make that work, that'd be fine too. #Closed

@Meir017
Copy link
Contributor Author

Meir017 commented Jan 23, 2019

@jcouv Done! #Closed

@alrz
Copy link
Member

alrz commented Jan 30, 2019

Looks like indentation is different from place to place? (2 spaces vs 4) #Closed

@Meir017
Copy link
Contributor Author

Meir017 commented Jan 30, 2019

hmm, looks like there are many name conflicts between the internal namespace and the public one.
for now, I'll just shorten the syntax without splitting into 2 files #Closed

@Meir017
Copy link
Contributor Author

Meir017 commented Jan 30, 2019

OK, now it's ready for review #Closed

@jcouv
Copy link
Member

jcouv commented Jan 30, 2019

internal override GreenNode SetDiagnostics(DiagnosticInfo[] diagnostics) => new OmittedTypeArgumentSyntax(this.Kind, this.omittedTypeArgumentToken, diagnostics, GetAnnotations());

nit: consider breaking fat arrows to the next line with an indent (as you did in the generated Test file), at least for long expressions.


Refers to: src/Compilers/CSharp/Portable/Generated/Syntax.xml.Internal.Generated.cs:1572 in c994637. [](commit_id = c994637, deletion_comment = False)

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 15)

@jcouv
Copy link
Member

jcouv commented Jan 30, 2019

Note: I've recently changed the code generator in master. I'd recommend that you rebase (so we can cleanly squash the PR once ready to merge), but you can merge if you prefer (then we'll just have to merge the PR instead of squashing it).

@jcouv
Copy link
Member

jcouv commented Feb 13, 2019

Thanks. @dotnet/roslyn-compiler for a second review of this community PR.

1 similar comment
@jcouv
Copy link
Member

jcouv commented Mar 18, 2019

Thanks. @dotnet/roslyn-compiler for a second review of this community PR.

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

LGTM (commit 16)

@Meir017
Copy link
Contributor Author

Meir017 commented Mar 19, 2019

@333fred Just merged from master to resolve the conflicts and then ran the generators again to fix the generated code

@jcouv jcouv added this to the 16.1.P1 milestone Mar 19, 2019
@jcouv jcouv merged commit b429811 into dotnet:master Mar 19, 2019
@jcouv
Copy link
Member

jcouv commented Mar 19, 2019

Thanks @Meir017!

@Meir017 Meir017 deleted the more-changes branch March 20, 2019 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants