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

Respect user settings around 'Blocks' when adding parameter null checks. #19426

Merged
merged 6 commits into from
May 11, 2017

Conversation

CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented May 11, 2017

Fixes #19172
Followup to #19424

Copy link
Member

@sharwell sharwell left a comment

Choose a reason for hiding this comment

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

I'm not convinced this is the right answer. It seems to be applied to all cases where the simplifier annotation is used, and not just to the null check case described in the original issue. In particular, it doesn't account for cases which I find important:

  1. if statements which include an else clause
  2. Child statements which span more than one line

At minimum this will need several new tests covering the simplification behavior change.

@CyrusNajmabadi
Copy link
Member Author

CyrusNajmabadi commented May 11, 2017

It seems to be applied to all cases where the simplifier annotation is used, and not just to the null check case described in the original issue

Yes. Any time someone uses the generator (which is what puts on the SimplifierAnnotation) and creates a single-statement block, that single-statement block will be simplified if that's what the user preference is.

@sharwell
Copy link
Member

@CyrusNajmabadi I'm worried that somewhere something is going to start doing this to code, and I'd rather be ignoring the setting than having that happen. At this point I'm not comfortable with the potential impact of this change given the current inability to specify finer-grain control over the behavior.

@CyrusNajmabadi
Copy link
Member Author

@sharwell We could certainly tweak the 'prefer block' preference to have another option for single-lines. but, right now, this is just making it so that our codegen APIs respect user preferences as currently stated.

@CyrusNajmabadi
Copy link
Member Author

Adding tests for the other constructs.

@CyrusNajmabadi
Copy link
Member Author

@DustinCampbell What do you think? I think it's appropriate for the syntax-generator to be updated to respect the user's option here. I would also support future work to have a more fine-grained option. like "Use braces for multiline nested statements" (actual text to be massage later).

@CyrusNajmabadi
Copy link
Member Author

retest windows_release_vs-integration_prtest please

@CyrusNajmabadi
Copy link
Member Author

Tagging @Pilchie

@Pilchie
Copy link
Member

Pilchie commented May 11, 2017

I'm with Cyrus here, I think we can take this and then consider extending the flexibility of the option.

@CyrusNajmabadi
Copy link
Member Author

Ok. Merging.

@CyrusNajmabadi
Copy link
Member Author

tagging @MattGertz

Customer scenario

This makes one of our new features better respect user codestyle-options. Specifically, when adding a null check, we'll respect the user's preferences around if that null check should use a { } block in it.

Bugs this fixes:

Fixes #19172

Workarounds, if any

None.

Risk

Low.

Performance impact

Low.

Is this a regression from a previous update?

No.

Root cause analysis:

Didn't validate all new features against all codestyle permutations.

How was the bug found?

Customer report.

@MattGertz
Copy link
Contributor

Approving per Kevin's comment. (The scenario meets the bar at any rate.) We should file an issue to track extending it, though.

@CyrusNajmabadi CyrusNajmabadi merged commit 3506fca into dotnet:master May 11, 2017
@CyrusNajmabadi CyrusNajmabadi deleted the blockSimplification branch May 11, 2017 23:40
@CyrusNajmabadi
Copy link
Member Author

I'll open hte issue.

@CyrusNajmabadi
Copy link
Member Author

#19451

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.

5 participants