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.

Sign up for GitHub

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

Jump to bottom

Code cleanup from temporary enabling .NET analyzers #601

Merged
merged 5 commits into from
Aug 5, 2024
Merged

Code cleanup from temporary enabling .NET analyzers #601

merged 5 commits into from
Aug 5, 2024

Conversation

ErikEJ
Copy link
Collaborator

@ErikEJ ErikEJ commented Aug 2, 2024 •
edited
Loading

Also: removed .NET 7 support and updated readme to 2.8.1

<AnalysisMode>all</AnalysisMode>
    <EnableNETAnalyzers>true</EnableNETAnalyzers>
    <AnalysisLevel>latest</AnalysisLevel>
    <EnforceCodeStyleInBuild>true</EnforceCodeStyleInBuild>

ErikEJ added 3 commits August 2, 2024 13:57
@ErikEJ
Code cleanup from temporary enabling .NET analyzers;
d054755 
    <AnalysisMode>all</AnalysisMode>
    <EnableNETAnalyzers>true</EnableNETAnalyzers>
    <AnalysisLevel>latest</AnalysisLevel>
    <EnforceCodeStyleInBuild>true</EnforceCodeStyleInBuild>
@ErikEJ
fix build
0d18b1a
@ErikEJ
remove .NET 7
5b9d505
@ErikEJ ErikEJ changed the title Code cleanup from temporary enabling .NET analyzers; Code cleanup from temporary enabling .NET analyzers Aug 2, 2024
@ErikEJ ErikEJ requested review from jmezach and jeffrosenberg August 2, 2024 12:21
@jmezach
Copy link
Member

jmezach commented Aug 2, 2024

Is there any reason not to keep these analyzers on?

@ErikEJ
Copy link
Collaborator Author

ErikEJ commented Aug 2, 2024

Is there any reason not to keep these analyzers on?

@jmezach Yes, they produce a lot of warnings from the module code (that we cannot modify)

@jmezach
Copy link
Member

jmezach commented Aug 2, 2024

You mean the sqltoolsservice submodule? Yeah, I've been wondering if we can somehow get rid of that dependency as we haven't been able to update it for quite a while now.

@ErikEJ
Copy link
Collaborator Author

ErikEJ commented Aug 2, 2024

Getting rid of it would be great - looks like there is also ways to exclude files and/or folder from analysis dotnet/roslyn#3705

@ErikEJ
Copy link
Collaborator Author

ErikEJ commented Aug 2, 2024

We could maybe log an improvement issue to get this done?

@jmezach
Copy link
Member

jmezach commented Aug 2, 2024

Sounds good to me

@ErikEJ
Copy link
Collaborator Author

ErikEJ commented Aug 2, 2024

Logged #602

jeffrosenberg
jeffrosenberg approved these changes Aug 5, 2024
View reviewed changes
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.

A few minor comments... I'm going to go ahead and approve, since I probably won't be able to look back at this PR for 12 hours or so

.github/workflows/main.yml Show resolved Hide resolved
src/MSBuild.Sdk.SqlProj/MSBuild.Sdk.SqlProj.csproj Show resolved Hide resolved
src/DacpacTool/PackageBuilder.cs Show resolved Hide resolved
src/DacpacTool/PackageDeployer.cs Show resolved Hide resolved
@ErikEJ ErikEJ merged commit f9fb9f2 into rr-wfm:master Aug 5, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeffrosenberg jeffrosenberg jeffrosenberg approved these changes

@jmezach jmezach jmezach approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ErikEJ @jmezach @jeffrosenberg