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

Populate switch tweaks #10846

Merged
merged 26 commits into from
May 7, 2016
Merged

Conversation

CyrusNajmabadi
Copy link
Member

This change tweaks 'Populate Switch' to provide from 1-3 fixes to the user depending on the state of the switch. The potential fixes provided are:

Add missing cases.
Add missing 'default' case
Add both

@Hosch250
Copy link
Contributor

Why did you PR to Future instead of Master? My PR was merged into Master, so shouldn't it be corrected there and be auto-PR'ed to Future?

@CyrusNajmabadi
Copy link
Member Author

Because IOperatoin has been disabled in Master :)

@mattwar
Copy link
Contributor

mattwar commented Apr 25, 2016

@dotnet-bot retest prtest/lin/dbg/unit32 please
// Previous failure: http://dotnet-ci.cloudapp.net/job/roslyn_prtest_lin_dbg_unit32/6507/
// Retest reason:

@Hosch250
Copy link
Contributor

OK. I hope this gets worked in before it is released: it is quite an improvement over my first version.

@CyrusNajmabadi
Copy link
Member Author

We're discussing what to do. :)

@CyrusNajmabadi
Copy link
Member Author

@dotnet/roslyn-ide please review.

@CyrusNajmabadi
Copy link
Member Author

We've decided we're going to do all the work here out of 'future'. We'll be disabling this in master and shipping this out of future (with IOperations) instead. Thanks!

@rchande
Copy link
Contributor

rchande commented Apr 29, 2016

👍

@CyrusNajmabadi
Copy link
Member Author

tagging @dotnet/roslyn-ide For signoff.

@CyrusNajmabadi
Copy link
Member Author

test vsi please

@jmarolf
Copy link
Contributor

jmarolf commented May 4, 2016

👍

@CyrusNajmabadi
Copy link
Member Author

retest this please

@CyrusNajmabadi
Copy link
Member Author

retest vsi please

@DustinCampbell
Copy link
Member

adding @mattwar for SyntaxGenerator API additions

@DustinCampbell
Copy link
Member

change looks righteous though 👍

@mattwar
Copy link
Contributor

mattwar commented May 5, 2016

what are the missing tokens for?

@CyrusNajmabadi
Copy link
Member Author

So that if you add switch sections to switch (foo) you get switch (foo) { case ... } not switch (foo) case ...

This enables someone to easily just do: switch (foo)<ctrl-dot><add missing cases> and have a fully formed switch they can use.

@mattwar
Copy link
Contributor

mattwar commented May 5, 2016

in case you are operating on a code fragment? I see, since you've only just typed the first part and you want to auto generate the cases? Couldn't you just add the token directly without the rewriter?

@Hosch250
Copy link
Contributor

Hosch250 commented May 5, 2016

I haven't tried this, but you could probably just do CSharpSyntaxFactory.Block({newNodesFromGenerator}) instead of modifying the generator.

@CyrusNajmabadi
Copy link
Member Author

Couldn't you just add the token directly without the rewriter?

I could... but then i'd have to write logic specifically to handle the open and close tokens being missing. I just decided i would write the helper once and then would have it whenver i needed it.

@CyrusNajmabadi
Copy link
Member Author

CSharpSyntaxFactory.Block({newNodesFromGenerator})

'Block' won't generate the write type of node for a switch statement unfortunately.

@CyrusNajmabadi
Copy link
Member Author

test vsi please

@CyrusNajmabadi
Copy link
Member Author

test vsi please

@CyrusNajmabadi CyrusNajmabadi merged commit 32715fe into dotnet:future May 7, 2016
@CyrusNajmabadi CyrusNajmabadi deleted the populateSwitch2 branch May 7, 2016 10:52
@CyrusNajmabadi CyrusNajmabadi restored the populateSwitch2 branch May 30, 2016 21:04
@CyrusNajmabadi CyrusNajmabadi deleted the populateSwitch2 branch January 25, 2020 00:30
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

7 participants