Skip to content

Enable CA1822 in Visual Studio layer#50569

Merged
mavasani merged 28 commits intodotnet:mainfrom
Youssef1313:patch-43
Jan 20, 2022
Merged

Enable CA1822 in Visual Studio layer#50569
mavasani merged 28 commits intodotnet:mainfrom
Youssef1313:patch-43

Conversation

@Youssef1313
Copy link
Copy Markdown
Member

The issues stated were fixed and closed.

@ghost ghost added the Area-IDE label Jan 17, 2021
@Youssef1313 Youssef1313 requested a review from a team as a code owner January 18, 2021 09:18
@Youssef1313 Youssef1313 marked this pull request as draft January 18, 2021 09:39
.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
Copy Markdown
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

public class ResetInteractiveTests
{
private string WorkspaceXmlStr =>
private const string WorkspaceXmlStr =
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@jinujoseph
Copy link
Copy Markdown
Contributor

Thanks for the contribution @Youssef1313, Blocking this temporarily as we discuss with the team.

Base automatically changed from master to main March 3, 2021 23:53
@Youssef1313
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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