Skip to content

Update templates to use file scoped namespace declarations #34220

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 6 commits into from
Jul 15, 2021

Conversation

DamianEdwards
Copy link
Member

@DamianEdwards DamianEdwards commented Jul 8, 2021

Depends on dotnet/roslyn#54763 being merged and flowing to SDK before dotnet/aspnetcore can get an SDK with support for this feature in the compiler.

Contributes to #33947 #33948

@DamianEdwards DamianEdwards added the blocked The work on this issue is blocked due to some dependency label Jul 8, 2021
@DamianEdwards DamianEdwards added this to the 6.0-preview7 milestone Jul 8, 2021
@DamianEdwards DamianEdwards self-assigned this Jul 8, 2021
@DamianEdwards DamianEdwards requested review from Pilchie and a team as code owners July 8, 2021 23:46
@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jul 8, 2021
Copy link
Contributor

@pranavkm pranavkm left a comment

Choose a reason for hiding this comment

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

As long as it compiles!

{
HttpContext.VerifyUserHasAnyAcceptedScope(scopeRequiredByApi);

using var response = await _downstreamWebApi.CallWebApiForUserAsync("DownstreamApi").ConfigureAwait(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

@davidfowl does this trigger you?

Copy link
Member Author

Choose a reason for hiding this comment

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

For the record, I didn't change that, that's simply what was there already 🙃

Copy link
Member

@davidfowl davidfowl Jul 9, 2021

Choose a reason for hiding this comment

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

What is this................

Copy link
Member

Choose a reason for hiding this comment

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

Why is it using configure await, I have so many questions.

Copy link
Contributor

Choose a reason for hiding this comment

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

This was part of the Identity.Web work: #24024

@DamianEdwards
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@DamianEdwards DamianEdwards force-pushed the damianedwards/33947-file-scoped-namespaces branch 2 times, most recently from c9428c1 to f04c337 Compare July 12, 2021 16:18
@DamianEdwards
Copy link
Member Author

So turns out the right branch of Roslyn wasn't flowing into the SDK so we would've been waiting a while for the feature to appear. That's now been fixed so once code flow results in a new SDK being produced and flowed to installer which in turn flows to /aspnetcore (right?) we can finally get this change in.

@DamianEdwards DamianEdwards force-pushed the damianedwards/33947-file-scoped-namespaces branch from c713da4 to 03dadf8 Compare July 14, 2021 19:12
@dougbu
Copy link
Contributor

dougbu commented Jul 14, 2021

@DamianEdwards this is going to conflict w/ the move to https://github.com/dotnet/spa-templates and my current work on the second phase of #32027 (using the new repo as a submodule in this one). Any chance this could wait a day so that your changes to the Spa template could be made in the new repo❔

@DamianEdwards
Copy link
Member Author

@DamianEdwards we're trying to get this in for preview.7, is your change targeting preview.7 too?

@dougbu
Copy link
Contributor

dougbu commented Jul 14, 2021

we're trying to get this in for preview.7, is your change targeting preview.7 too?

Yes❕ I'll put up the "phase 2" PR shortly after lunch I hope.

@DamianEdwards
Copy link
Member Author

@dougbu so should I just undo the changes to the SPA templates given they're moving?

@dougbu
Copy link
Contributor

dougbu commented Jul 14, 2021

@dougbu so should I just undo the changes to the SPA templates given they're moving?

That'll work for me if it works for you.

@DamianEdwards DamianEdwards removed the blocked The work on this issue is blocked due to some dependency label Jul 14, 2021
@DamianEdwards DamianEdwards requested a review from Tratcher as a code owner July 14, 2021 23:36
@DamianEdwards
Copy link
Member Author

Disabled the warnings that started showing up with the new SDK build (thanks to the new interpolated string builders language feature) on advice from @pranavkm.

New compiler now makes this appear when using a StringBuilder with string interpolation. I'll log an issue to follow up.
@DamianEdwards DamianEdwards force-pushed the damianedwards/33947-file-scoped-namespaces branch from b7cdccf to 49dce32 Compare July 15, 2021 00:07
@DamianEdwards DamianEdwards enabled auto-merge (squash) July 15, 2021 00:29
@DamianEdwards DamianEdwards merged commit 25767de into main Jul 15, 2021
@DamianEdwards DamianEdwards deleted the damianedwards/33947-file-scoped-namespaces branch July 15, 2021 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants