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

Support NuGet based code analyzers #625

Merged
merged 17 commits into from
Oct 1, 2024
Merged

Conversation

ErikEJ
Copy link
Collaborator

@ErikEJ ErikEJ commented Sep 24, 2024

fixes #569

This supports the "official" way to pack SQL analyzers.

  • Docs update: Show package references
  • Docs update: Mention netstandard2.1 requirement
  • Docs update: Remove support for old way of adding rules TBD

For an example of an analyzer .csproj see this

Wonder if we should remove support for our current way to add analyzers? - I am tempted to.

Notice that the Test project has been updated to target netstandard2.1 in order for restore of the rule dll to work.

@ErikEJ
Copy link
Collaborator Author

ErikEJ commented Sep 24, 2024

Will also add docs update if we agree to merge this.

@ErikEJ
Copy link
Collaborator Author

ErikEJ commented Sep 24, 2024

@jmezach Thoughts on: Wonder if we should remove support for our current way to add analyzers? - I am tempted to.

@jmezach
Copy link
Member

jmezach commented Sep 24, 2024

What do you mean exactly with our current way of adding analyzers?

@ErikEJ
Copy link
Collaborator Author

ErikEJ commented Sep 24, 2024

@jmezach I mean this part of the docs:

You can also bring your own rules. For an example of custom rules, see this repository.

To use custom rules, place the rule .dll files in a Rules folder in the project, and add them as Content items:

  <ItemGroup>
    <Content Include="Rules\My.Own.Rules.dll" />
  </ItemGroup>

@ErikEJ
Copy link
Collaborator Author

ErikEJ commented Sep 24, 2024

@jmezach In other words, I suggest we revert this PR: #533 (and update the docs of course)

@jmezach
Copy link
Member

jmezach commented Sep 24, 2024

Yeah, I think that makes sense. We might want to explain how folks could create a NuGet package with their custom rules though.

@ErikEJ
Copy link
Collaborator Author

ErikEJ commented Sep 24, 2024

We might want to explain how folks could create a NuGet package with their custom rules though.

Yes, working on the docs now!

Remove file based "bring your own rules" support
@ErikEJ
Copy link
Collaborator Author

ErikEJ commented Sep 25, 2024

@jmezach I have gotten concerned that we are now reducing the number of active rules from 70+ to 15 for a user coming from a previous version of the SDK.

Could we add a warning if no Analyzers are referenced during analysis? And give the warning a code, so it can be overriden?

@jmezach
Copy link
Member

jmezach commented Sep 25, 2024

Yeah, I think that would make sense to do.

@ErikEJ
Copy link
Collaborator Author

ErikEJ commented Sep 25, 2024

image

@jmezach
Copy link
Member

jmezach commented Sep 25, 2024

LGTM :)

@ErikEJ
Copy link
Collaborator Author

ErikEJ commented Sep 25, 2024

@jmezach PR updated with the warning and some more logging info

@ErikEJ
Copy link
Collaborator Author

ErikEJ commented Sep 26, 2024

@jmezach any thing else you would like me to address?

Copy link
Collaborator

@jeffrosenberg jeffrosenberg left a comment

Choose a reason for hiding this comment

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

LGTM, just left a few small corrections on the README

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
ErikEJ and others added 3 commits September 26, 2024 14:59
Co-authored-by: Jeff Rosenberg <rosenberg.jeff@gmail.com>
Co-authored-by: Jeff Rosenberg <rosenberg.jeff@gmail.com>
Co-authored-by: Jeff Rosenberg <rosenberg.jeff@gmail.com>
@ErikEJ
Copy link
Collaborator Author

ErikEJ commented Sep 26, 2024

@jeffrosenberg Thanks!

@ErikEJ ErikEJ requested a review from jmezach September 27, 2024 14:30
@ErikEJ
Copy link
Collaborator Author

ErikEJ commented Sep 28, 2024

@jmezach Wonder if I should update the docs in general with this?

<TargetFramework>netstandard2.1</TargetFramework>

@jmezach
Copy link
Member

jmezach commented Sep 30, 2024

My apologies, I've been out sick for a couple of days, hence the radio silence.

@ErikEJ If targeting netstandard2.1 is a requirement for being able to reference the rules packages, then yes, maybe we should update the docs. It doesn't have a big impact anyway. We should probably also update the templates for that.

@ErikEJ
Copy link
Collaborator Author

ErikEJ commented Sep 30, 2024

@jmezach Glad that you are well again!

I have updated the readme, the templates were already updated

Copy link
Member

@jmezach jmezach left a comment

Choose a reason for hiding this comment

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

Looks good overall, just a couple of tweaks I think

@ErikEJ
Copy link
Collaborator Author

ErikEJ commented Oct 1, 2024

@jmezach If we default to having Code Analysis always on, I assume we need to update the readme as well?

@ErikEJ
Copy link
Collaborator Author

ErikEJ commented Oct 1, 2024

@jmezach But maybe I misunderstood your review comment?

@ErikEJ
Copy link
Collaborator Author

ErikEJ commented Oct 1, 2024

Thanks @jmezach and @jeffrosenberg

I will also update my blog post here, when 2.9.0 is released:

https://erikej.github.io/dacfx/codeanalysis/sqlserver/2024/04/02/dacfx-codeanalysis.html

@ErikEJ ErikEJ merged commit f7d9b0a into rr-wfm:master Oct 1, 2024
10 checks passed
@ErikEJ
Copy link
Collaborator Author

ErikEJ commented Oct 1, 2024

@jmezach Can you assist with a release of 2.9.0 🙏 - TIA

@jmezach
Copy link
Member

jmezach commented Oct 1, 2024

Release pipeline should be running now

@jmezach
Copy link
Member

jmezach commented Oct 1, 2024

2.9.0 is up on NuGet

@jmezach
Copy link
Member

jmezach commented Oct 1, 2024

Do you want to write release notes for this?

@ErikEJ
Copy link
Collaborator Author

ErikEJ commented Oct 1, 2024

I can do that yes

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.

Consume rules from a Nuget package
3 participants