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

Add style option (and UI) for preferring file scoped namespaces #55043

Merged
merged 50 commits into from
Jul 26, 2021

Conversation

CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented Jul 22, 2021

UI looks like this:

image

and

image

This is exposed through the lightbulb as both an analyzer and a refactoring (similar ot how prefer-expression-body works). The analyzer will enforce the code style if set. The refactoring will allow you to switch to the either direction if you prefer. It looks like this:

image

image

Todo:

  • Features taht generate code should respect this style and should generate a file-scoped-namespace if desired
  • Tests

Relates to test plan: #49000

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner July 22, 2021 05:53
Copy link
Member

@Youssef1313 Youssef1313 left a comment

Choose a reason for hiding this comment

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

I think both automation object and editorconfig UI need to be updated for this new option.


// if the diagnostic is hidden, show it anywhere from the `namespace` keyword through the name.
// otherwise, if it's not hidden, just squiggle the name.
var diagnosticLocation = severity.WithDefaultSeverity(DiagnosticSeverity.Hidden) == ReportDiagnostic.Hidden
Copy link
Member

Choose a reason for hiding this comment

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

I'm worried the severity variable here only reflects the option = value:severity syntax but not reflecting the dotnet_diagnostic.ID.severity = severity syntax.

Copy link
Member Author

Choose a reason for hiding this comment

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

so this follows what we do with use-expression-body. i'm going to stick with this pattern under teh presumption that it's correct. if not, we shoudl fix it for all of these. i do not want to have multiple different patterns at the same time :)

Copy link
Member

Choose a reason for hiding this comment

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

Tagging @mavasani to confirm if there is a problem here in which case I'll file an issue.


var option = optionSet.GetOption(CSharpCodeStyleOptions.PreferFileScopedNamespace);
var userPrefersRegularNamespaces = !option.Value;
var analyzerDisabled = option.Notification.Severity == ReportDiagnostic.Suppress;
Copy link
Member

Choose a reason for hiding this comment

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

Same comment here. Notification.Severity most likely doesn't reflect the dotnet_diagnostic..... syntax

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 reaklly do not want our features to deviate from each other. until there is a suitable new style we should keep our code consistent. Otherwise we'll just have a hodgepodge of impls with varying patterns that people have to udnerstand. If and when we have some new pattern that everything should follow, we can move all our code to that at once.

@davidwengier
Copy link
Contributor

I haven't looked at the code yet, but is there a more descriptive name than "regular namespace"? Like "block-scoped namespace", or "block bodied namespace" to match how we talk about methods.

If new C# templates are going to use top level statements in .NET 6, I could see them using file-scoped namespaces in the same timeframe, or in the future, so that word "regular" will tend to lose its meaning.

@CyrusNajmabadi
Copy link
Member Author

CyrusNajmabadi commented Jul 22, 2021

I haven't looked at the code yet, but is there a more descriptive name than "regular namespace"? Like "block-scoped namespace",

Yes, this is a good point. I will use 'block-bodied' or 'block-scoped' instead :)

Copy link
Member

@Youssef1313 Youssef1313 left a comment

Choose a reason for hiding this comment

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

Note: #55043 (review) is not addressed yet

@CyrusNajmabadi
Copy link
Member Author

@Youssef1313 I'm not doing editorconfig UI until #52343 is addressed. There are just too many duplicated strings in random places that have to be resolved before going that path.

Copy link
Contributor

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

:shipit:

@CyrusNajmabadi CyrusNajmabadi merged commit dbec250 into dotnet:main Jul 26, 2021
@ghost ghost added this to the Next milestone Jul 26, 2021
@CyrusNajmabadi CyrusNajmabadi deleted the fileScopedNamespaceStyle branch July 26, 2021 16:46
@CyrusNajmabadi
Copy link
Member Author

@jmarolf can you either add the EditorConfig side of this, or let me know when we can add EditorConfig settings without duplication? Thanks!

@AraHaan
Copy link
Member

AraHaan commented Jul 26, 2021

I second this so that way source generators (like CsWin32) can pick it up and then generate using file scoped namespace.

Bonus if something like this can be done to global usings as well too as well as a way to set what file it should place the global usings in (so source generators that generate code with usings could decide if it should add those usings or not add them and let the user manually add them to their global usings file).

@CyrusNajmabadi
Copy link
Member Author

@AraHaan as mentioned already, a source generator can see these values when it executes and can generate accordingly.

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

Successfully merging this pull request may close these issues.

8 participants