Skip to content

Adding some IDE code fix tests for async using & foreach#26632

Merged
jcouv merged 2 commits intodotnet:features/async-streamsfrom
Neme12:async-streams-makeMethodAsync
May 4, 2018
Merged

Adding some IDE code fix tests for async using & foreach#26632
jcouv merged 2 commits intodotnet:features/async-streamsfrom
Neme12:async-streams-makeMethodAsync

Conversation

@Neme12
Copy link
Copy Markdown
Contributor

@Neme12 Neme12 commented May 4, 2018 •
edited
Loading

I discovered that these features already work so I just added the tests.

Tagging @jcouv this is for your feature branch

@Neme12 Neme12 requested a review from a team as a code owner May 4, 2018 20:23
@etbyrd etbyrd added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label May 4, 2018
jcouv
jcouv approved these changes May 4, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks

@jcouv jcouv added the Area-IDE label May 4, 2018
@jcouv jcouv added this to the 16.0 milestone May 4, 2018
@jcouv jcouv self-assigned this May 4, 2018
@jcouv
Copy link
Copy Markdown
Member

jcouv commented May 4, 2018

@dotnet/roslyn-ide for a second review (test-only change). Thanks

@Neme12
Copy link
Copy Markdown
Contributor Author

Neme12 commented May 4, 2018 •
edited
Loading

Does this need review from the IDE team?
edit: You were faster than me, thanks.

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

CyrusNajmabadi commented May 4, 2018

Note: in general, our requirements for a test-only change are much weaker. Tagging @jinujoseph for her thoughts here. I think it's fine to just get a single review for safe and clear test-only changes like these.

@jinujoseph
Copy link
Copy Markdown
Contributor

jinujoseph commented May 4, 2018

yes, single review from IDE team is good.

@Neme12
Copy link
Copy Markdown
Contributor Author

Neme12 commented May 4, 2018

yes, single review from IDE team is good.

Can I get a buddy from IDE then please? Thank you

@jinujoseph
Copy link
Copy Markdown
Contributor

jinujoseph commented May 4, 2018

I just reviewed it , good from IDE

@Neme12
Copy link
Copy Markdown
Contributor Author

Neme12 commented May 4, 2018

@jinujoseph Thanks, I commented at the same time and didn't notice (again).

@jcouv jcouv merged commit e6785b8 into dotnet:features/async-streams May 4, 2018
@jcouv
Copy link
Copy Markdown
Member

jcouv commented May 4, 2018

Thanks much, @Neme12!

@Neme12 Neme12 deleted the async-streams-makeMethodAsync branch May 4, 2018 22:43
@Neme12
Copy link
Copy Markdown
Contributor Author

Neme12 commented Oct 7, 2018

@jcouv Note: I think you should be able to check the third checkbox in this list as this PR took care of testing that (from #24037):

  • Code fixers:

    • fix foreach (by adding/removing await) depending on the collection type
    • fix method declaration depending on presence/absence of yield or await
    • using foreach await in non-async method should offer to convert the method to async

@jcouv jcouv mentioned this pull request Oct 7, 2018
Closed
@jcouv
Copy link
Copy Markdown
Member

jcouv commented Oct 7, 2018

Cool!
Checked that box. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jcouv jcouv jcouv approved these changes

+2 more reviewers

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

@jinujoseph jinujoseph jinujoseph approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jcouv jcouv

Labels

Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee.

Projects

None yet

Milestone

16.0

Development

Successfully merging this pull request may close these issues.

6 participants

@Neme12 @jcouv @CyrusNajmabadi @jinujoseph @sharwell @etbyrd