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

Remove unnecessary dependency on MS.CA.VB package #29986

Merged
merged 1 commit into from
Jan 6, 2023
Merged

Remove unnecessary dependency on MS.CA.VB package #29986

merged 1 commit into from
Jan 6, 2023

Conversation

DoctorKrolic
Copy link
Contributor

This PR does not fix any bugs or add new features and is very small in size, so I think, it doesn't need an issue to be opened first. I just notecied, that for whatever reason analyzer tests have a dependency on a VB package, while the analyzers themselves are for C# only. I verified, that removing this dependency doesn't break test execution locally. If there is some mysterious sense in having this dependency, I won't regret if the PR gets closed. It actually took more time to setup EF repo on my machine than the time taken by the actual change...

@roji
Copy link
Member

roji commented Jan 5, 2023

@DoctorKrolic at least in principle, the analyzers are supposed to work for VB code as well (e.g. warn for internal API usage). This should be validated by tests, though I don't think we've ever gone through the trouble of writing them.

@DoctorKrolic
Copy link
Contributor Author

Both current analizers are C#-only:

[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class InternalUsageDiagnosticAnalyzer : DiagnosticAnalyzer

[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class UninitializedDbSetDiagnosticSuppressor : DiagnosticSuppressor

The analizer project doesn't target VB compiler package at all.
I mean, why should we pay cost of downloading and linking package that is not actually used for every local/CI test build?

@roji
Copy link
Member

roji commented Jan 5, 2023

I mean, why should we pay cost of downloading and linking package that is not actually used for every local/CI test build?

I agree the package isn't needed, but don't you think that's very negligible, considering this is a test-only dependency anyway?

@DoctorKrolic
Copy link
Contributor Author

don't you think that's very negligible

Yes, but the cost of review is basically 0 as well. All CI is green, so I can be trusted now, that this doesn't break anything)

@roji
Copy link
Member

roji commented Jan 5, 2023

Sure. But we generally don't do merge commits; can you please rebase your change on the latest main, as a single commit without merge branches? You can force-push that to this PR's branch.

@DoctorKrolic
Copy link
Contributor Author

@roji You can probably merge this PR

Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

Thanks

@roji roji merged commit fc8478f into dotnet:main Jan 6, 2023
@DoctorKrolic DoctorKrolic deleted the remove-vb-dependency branch January 6, 2023 18:06
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.

None yet

3 participants