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

Mvc.Analyzers are no longer imported in 3.0 projects #4373

Closed
natemcmaster opened this issue Dec 3, 2018 · 12 comments
Closed

Mvc.Analyzers are no longer imported in 3.0 projects #4373

natemcmaster opened this issue Dec 3, 2018 · 12 comments
Assignees
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates bug This issue describes a behavior which is not expected - a bug. Done This issue has been fixed

Comments

@natemcmaster
Copy link
Contributor

As a part of #4061 and #3612, we removed Microsoft.AspNetCore.Mvc.Analyzers from Microsoft.AspNetCore.App since we do not plan to continue to ship AspNetCore.App as a metapackage. We should find a way to re-add these analyzers to netcoreapp3.0 projects.

Ideas

  • Ship as a PackageReference. Projects must reference as a package
    • Add the PackageRef to templates?
    • Make the PackageRef implicit in the WebSDK?
  • Or, work with the SDK team to ship analyzers in-box without a package reference.

cc @davidfowl @pranavkm @mkArtakMSFT

@natemcmaster natemcmaster added bug This issue describes a behavior which is not expected - a bug. area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Dec 3, 2018
@mkArtakMSFT mkArtakMSFT added the Needs: Spec Indicates that a spec defining user experience is required label Dec 5, 2018
@mkArtakMSFT mkArtakMSFT added this to the 3.0.0-preview2 milestone Dec 5, 2018
@mkArtakMSFT
Copy link
Member

@danroth27, assigning this to you to define which way should we go here.

@danroth27
Copy link
Member

I think we shoudl have a single package reference in the templates for the analyzers that we think should be enabled by default.

@pranavkm
Copy link
Contributor

FYI @davidfowl

@mkArtakMSFT mkArtakMSFT assigned pranavkm and unassigned danroth27 Feb 12, 2019
@mkArtakMSFT mkArtakMSFT added 1 - Ready and removed Needs: Spec Indicates that a spec defining user experience is required PRI: 0 - Critical labels Feb 12, 2019
@mkArtakMSFT
Copy link
Member

mkArtakMSFT commented Feb 12, 2019

To summarize the discussion we had with @danroth27 and @glennc, we're moving forward with the following approach here:

  • Make the PackageRef implicit in the WebSDK

@davidfowl
Copy link
Member

Um what? why?

@DamianEdwards
Copy link
Member

I thought we'd said we'd look to get them integrated with the SDK?

@pranavkm
Copy link
Contributor

Spoke offline, here's the plan. We'll add ASP.NET Core analyzer binaries to the WebSdk and use the Analyzer MSBuild itemgroup to add these analyzers to the build. Like other implicitly enabled features, we'll have a property to turn these analyzers off.

@davidfowl
Copy link
Member

Doesn’t this mean that we need these packages in cache for offline storage? It also means we need a way to turn it off and change the version? And all of other things that come when you have implicit package references.

@pranavkm
Copy link
Contributor

pranavkm commented Feb 14, 2019

Doesn’t this mean that we need these packages in cache for offline storage?

There aren't packages - just binaries that ship in the SDK. Psuedo code:

<ItemGroup Condition="$(TargetFramework) &gt; 'netcoreapp3.0'' AND '$(DisableImplicitAspNetCoreAnalyzers)' != 'true'">
   <Analyzer Include="analyzers/$(TargetFramework)/*.dll" />
</ItemGroup>

@davidfowl
Copy link
Member

Ahhh clever. How do you turn them off?

@pranavkm
Copy link
Contributor

We would do a flag - see updated psuedo code.

@pranavkm
Copy link
Contributor

pranavkm commented Mar 5, 2019

Some thoughts as I was working on this: when we ship netcorepp3.1, we want to avoid shipping two copies of the analyzers particularly if they're source equivalent. However, we do want the ability to ship new analyzers without affect existing products i.e. updating the SDK should not result in new or different warnings in your existing applications.

One way to go about this would be to do a version detection in the analyzers i.e. look for the version of AspNetCore.App \ Mvc before lighting up a change. Using AssemblyVersionAttribute on one the AspNetCore assemblies should likely be sufficient to determine this.

pranavkm added a commit to dotnet/websdk that referenced this issue Mar 6, 2019
pranavkm added a commit to dotnet/websdk that referenced this issue Mar 7, 2019
pranavkm added a commit to dotnet/websdk that referenced this issue Mar 7, 2019
* Add AspNetCore analyzers to Web.Sdk

Fixes dotnet/aspnetcore#4373
@mkArtakMSFT mkArtakMSFT added Done This issue has been fixed and removed 2 - Working labels Mar 11, 2019
pranavkm added a commit that referenced this issue Apr 1, 2019
Since these analyzers are now available via the WebSDK, we no longer
need to ship these.

Follow up to #4373
pranavkm added a commit that referenced this issue Apr 1, 2019
Since these analyzers are now available via the WebSDK, we no longer
need to ship these.

Follow up to #4373
@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates bug This issue describes a behavior which is not expected - a bug. Done This issue has been fixed
Projects
None yet
Development

No branches or pull requests

6 participants