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 CA2009: Do not call ToImmutableCollection on an ImmutableCollection value #53141

Merged
merged 6 commits into from
Feb 22, 2022

Conversation

Youssef1313
Copy link
Member

No description provided.

@Youssef1313 Youssef1313 requested a review from a team as a code owner May 4, 2021 13:25
@Youssef1313 Youssef1313 marked this pull request as draft May 4, 2021 14:47
@Youssef1313 Youssef1313 marked this pull request as ready for review July 5, 2021 19:07
@Youssef1313 Youssef1313 closed this Jul 5, 2021
@Youssef1313 Youssef1313 reopened this Jul 5, 2021
@Youssef1313 Youssef1313 requested a review from a team as a code owner January 17, 2022 17:10
@@ -41,7 +41,7 @@ internal class OnAutoInsertHandler : AbstractStatelessRequestHandler<LSP.Documen
[ImportMany(LanguageNames.VisualBasic)] IEnumerable<IBraceCompletionService> visualBasicBraceCompletionServices)
{
_csharpBraceCompletionServices = csharpBraceCompletionServices.ToImmutableArray();
_visualBasicBraceCompletionServices = _visualBasicBraceCompletionServices.ToImmutableArray();
_visualBasicBraceCompletionServices = visualBasicBraceCompletionServices.ToImmutableArray();
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 like a bug!

@CyrusNajmabadi Any idea what this may have caused and how to test?

Copy link
Member

Choose a reason for hiding this comment

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

Nope. @dibarbet ?

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if there was a test that could have shown an issue here.

Copy link
Member Author

Choose a reason for hiding this comment

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

@CyrusNajmabadi I added a test. Haven't yet verified that it's failing before the change or passing after. Will test

Copy link
Member Author

Choose a reason for hiding this comment

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

The test I added is unrelated to this line. It's not a brace completion service. Will see if I can figure out a test.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't figure out a test. Most (if not all) of those VB services are unrelated to typing a new line, while OnAutoInsertHandler calls the services only if the character typed is a new line.

auto-merge was automatically disabled January 19, 2022 10:02

Head branch was pushed to by a user without write access

@Youssef1313
Copy link
Member Author

@CyrusNajmabadi @ryzngard There are unrelated test failures here. Can someone restart them? Thanks!

@RikkiGibson
Copy link
Contributor

Youssef, just wanted to confirm: the re-run button doesn't appear for you here in the Checks tab?

image

@Youssef1313
Copy link
Member Author

@RikkiGibson Yeah it doesn't (screenshot from another PR that has failing checks):

image

@RikkiGibson
Copy link
Contributor

@CyrusNajmabadi @Youssef1313 is this good to merge?

@Youssef1313
Copy link
Member Author

@RikkiGibson Yeah it should be.

@RikkiGibson RikkiGibson merged commit e220335 into dotnet:main Feb 22, 2022
@ghost ghost added this to the Next milestone Feb 22, 2022
@RikkiGibson
Copy link
Contributor

Thanks @Youssef1313!

@Youssef1313 Youssef1313 deleted the patch-17 branch February 22, 2022 20:24
@RikkiGibson RikkiGibson modified the milestones: Next, 17.2.P2 Mar 1, 2022
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.

4 participants