Skip to content

Add features to offer using conditional expressions ?: over explicit if-statement flows.#26236

Merged
ivanbasov merged 122 commits intodotnet:masterfrom
CyrusNajmabadi:useConditionalExpressionActualFeature
May 3, 2018
Merged

Add features to offer using conditional expressions ?: over explicit if-statement flows.#26236
ivanbasov merged 122 commits intodotnet:masterfrom
CyrusNajmabadi:useConditionalExpressionActualFeature

Conversation

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi commented Apr 18, 2018 •
edited by jcouv
Loading

Note: this PR depends on #25865. It should not be reviewed until that dependent PR is reviewed.

For example, with:

image

We offer:

image

Todo:

  • VB
  • Tests
  • Better formatting for large or multi-line clauses in the ?: expression.
  • Support this for if-statements with return statements in their bodies.

For the last one, this means recognizing:

if (expr) {
    return a;
} else {
    return b;
}

// as well as

if (expr) {
    return a;
}

return b;

and offering to convert to:

return expr ? a : b;

[jcouv edited] Fixes #26539

@CyrusNajmabadi
Initial UseConditional work.
514c920
@CyrusNajmabadi
Initial UseConditional work.
2a38b93
@CyrusNajmabadi
Working replacement.
7f82f3a
@CyrusNajmabadi
Extract common code.
96cd0ab
@CyrusNajmabadi
Make 'var' work properly in the simplifier service.
1561214
@CyrusNajmabadi
Move types to their own files.
78a5872
@CyrusNajmabadi
Provide a way to create types that are not simplified to 'var'.
ec650b9
@CyrusNajmabadi
Update test options.
05b6fb0
@CyrusNajmabadi
Update test.
11371db
@CyrusNajmabadi
Move CSharp code style code down to the Workspace layer.
919ab7a
@CyrusNajmabadi
Rename config value.
3f294cd
@CyrusNajmabadi
Add ui options.
40437e7
@CyrusNajmabadi
Add VB ui options.
3a6ce03
@CyrusNajmabadi
Add VB support
f5e476d
@CyrusNajmabadi
Strip trivia
4076b69
@CyrusNajmabadi
Initial stubs for 'use conditional for returns'
bfd91b2
@CyrusNajmabadi
Add VB impl.
5009d32
@CyrusNajmabadi
Only support processing of real trees.
6cd6ed3
@CyrusNajmabadi
Actually assign variable.
0a8219c
@CyrusNajmabadi
Don't assume a single variable.
7ab2623
@CyrusNajmabadi
Share more code.
186cd32
@CyrusNajmabadi
Fix shared code.
6ab3e4e
@CyrusNajmabadi
Add check.
d1ae6a4
@CyrusNajmabadi
Simplify code.
242b8eb
@CyrusNajmabadi
Only the analyzer needs to check the severity.
f81df9f
@CyrusNajmabadi
Generalize loc strings.
f56a680
@CyrusNajmabadi
remove static.
30c80a8
@CyrusNajmabadi
Remove unneeded methods.
23f8419
@CyrusNajmabadi
Do cheap checks first.
eda264e
@CyrusNajmabadi
Handle more cases.
0945c62
@ivanbasov
Copy link
Copy Markdown
Contributor

ivanbasov commented May 2, 2018

@CyrusNajmabadi , thank you! I prefer reviewing such great PR using CodeFlow not just browser view. However, CodeFlow supports up to 64 commits and the current PR contains 114 ones. Do you see any options to squeeze or to split the PR? Thanks!

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

CyrusNajmabadi commented May 2, 2018

Do you see any options to squeeze or to split the PR? Thanks!

That's problematic. I often branch off of these PRs, and squeezing/squashing things ends up making life very painful across those branches :(

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

CyrusNajmabadi commented May 2, 2018

Let me see if there's anything i can pull out here...

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

CyrusNajmabadi commented May 2, 2018

Darn... i'm not seeing anything conducive to pulling out. I kept this PR pretty self contained.

@KirillOsenkov Do you know why CodeFlow has a 64 commit limit? Is htat something that can be changed?

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

CyrusNajmabadi commented May 2, 2018

Actually, i guess i could make a squashed PR from anotehr branch and have you review that. i'd then make the changes here and merge that back into the other branch. let me try that out.

@KirillOsenkov
Copy link
Copy Markdown
Member

KirillOsenkov commented May 2, 2018

It’s not a CodeFlow problem, it’s GitHub rate limits.

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

CyrusNajmabadi commented May 2, 2018

@ivanbasov can you sign off on this one?

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

CyrusNajmabadi commented May 2, 2018

@KirillOsenkov That's a pity. Could it lazily be pulled in in the BG at some acceptable rate?

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

CyrusNajmabadi commented May 3, 2018

@jinujoseph Can this be merged in? @ivanbasov signed off on #26572 which is the squashed version of this. That was create for him because of codeflow issues. However, i would like to merge this in as i have several downstream branches and it will be a pain if they have to deal with the rebased/squashed PR.

@jcouv jcouv mentioned this pull request May 3, 2018
Closed
4 tasks
@jinujoseph
Copy link
Copy Markdown
Contributor

jinujoseph commented May 3, 2018

Approved to merge for 15.8.Preview2 ( once ivan signs of)

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

CyrusNajmabadi commented May 3, 2018

@ivanbasov can you sign off plz? Thanks!

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

CyrusNajmabadi commented May 3, 2018

@ivanbasov @jinujoseph for merging.

@ivanbasov ivanbasov merged commit 0949463 into dotnet:master May 3, 2018
@ivanbasov
Copy link
Copy Markdown
Contributor

ivanbasov commented May 3, 2018

Thank you very much, @CyrusNajmabadi, for you help!

@CyrusNajmabadi CyrusNajmabadi deleted the useConditionalExpressionActualFeature branch May 3, 2018 19:58
@sharwell sharwell added this to the 15.8 milestone Jun 7, 2018
@sharwell sharwell mentioned this pull request Jun 7, 2018
Closed
@CyrusNajmabadi CyrusNajmabadi mentioned this pull request Jun 14, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@ivanbasov ivanbasov ivanbasov approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ivanbasov ivanbasov

Labels

Approved to merge Area-IDE

Projects

None yet

Milestone

15.8

Development

Successfully merging this pull request may close these issues.

6 participants

@CyrusNajmabadi @jinujoseph @ivanbasov @KirillOsenkov @jaredpar @sharwell