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

Enable CA1822 in Visual Studio layer #50569

Merged
merged 28 commits into from
Jan 20, 2022
Merged

Conversation

Youssef1313
Copy link
Member

The issues stated were fixed and closed.

.editorconfig Outdated
# Additionally, there is a risk of accidentally breaking an internal API that partners rely on though IVT.
dotnet_diagnostic.CA1822.severity = suggestion
# There is a risk of accidentally breaking an internal API that partners rely on though IVT.
dotnet_diagnostic.CA1822.severity = warning
Copy link
Member Author

Choose a reason for hiding this comment

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

This looks redundant as it's already specified above, but it's required due to #50577

@@ -26,7 +26,7 @@ namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.Interactive.Commands
[UseExportProvider]
public class ResetInteractiveTests
{
private string WorkspaceXmlStr =>
private const string WorkspaceXmlStr =
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only change to const.

@mavasani
Copy link
Contributor

@Youssef1313 FYI that I already attempted this locally in the past, but there are lot of internal APIs in the VisualStudio layer that are used via IVTs by internal partner teams, so this poses a big risk of introducing breaking changes. I think this will be a risky change and would certainly require validation and approval from @sharwell and @jinujoseph

@Youssef1313
Copy link
Member Author

@Youssef1313 FYI that I already attempted this locally in the past, but there are lot of internal APIs in the VisualStudio layer that are used via IVTs by internal partner teams, so this poses a big risk of introducing breaking changes. I think this will be a risky change and would certainly require validation and approval from @sharwell and @jinujoseph

@mavasani I'm only changing private APIs. Any internal/public APIs are not touched.

@Youssef1313 Youssef1313 marked this pull request as ready for review January 18, 2021 19:23
@jinujoseph jinujoseph added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jan 19, 2021
Base automatically changed from master to main March 3, 2021 23:53
@Youssef1313
Copy link
Member Author

@jinujoseph @mavasani Any updates on this? Note that I'm only touching private APIs, which I think aren't exposed via InternalsVisibleTo, so there shouldn't be a problem.

@mavasani
Copy link
Contributor

@Youssef1313 thanks, given it is only for private APIs, it should be fine. I’ll review the PR. Thanks for your patience.

@ghost ghost added the Needs UX Triage label Jan 14, 2022
@Youssef1313
Copy link
Member Author

@mavasani This is passing CI now.

@mavasani mavasani merged commit 759b3ca into dotnet:main Jan 20, 2022
@ghost ghost added this to the Next milestone Jan 20, 2022
@Youssef1313 Youssef1313 deleted the patch-43 branch January 20, 2022 05:46
@RikkiGibson RikkiGibson modified the milestones: Next, 17.2.P1 Feb 4, 2022
@ryzngard ryzngard added UX Review Not Required UX Review Not Required and removed Needs UX Triage labels Apr 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE Blocked Community The pull request was submitted by a contributor who is not a Microsoft employee. UX Review Not Required UX Review Not Required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants