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

Fix ValueGeneratedOnUpdate in scaffolding #18619

Merged
merged 1 commit into from
Oct 29, 2019
Merged

Conversation

roji
Copy link
Member

@roji roji commented Oct 28, 2019

And add some tests.

Fixes #18579

@roji roji force-pushed the ScaffoldOnUpdate branch 2 times, most recently from 1b86c4a to 4f0f24b Compare October 28, 2019 16:33
@@ -703,28 +703,16 @@ private void GenerateProperty(IProperty property, bool useDataAnnotations)
if (((IConventionProperty)property).GetValueGeneratedConfigurationSource().HasValue
&& RelationalValueGenerationConvention.GetValueGenerated(property) != valueGenerated)
{
string methodName;
switch (valueGenerated)
var methodName = valueGenerated switch
Copy link
Contributor

Choose a reason for hiding this comment

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

switch block.

Copy link
Member Author

Choose a reason for hiding this comment

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

Any reason here? We agreed to use it except for complex situations, and this seems as simple as it gets. If we don't use it here we basically don't use it anywhere, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

3 line ternary as a return of one of the statement. That is complex enough.

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 is not how I understand the decision made by the team, and basically invalidates the use of this construct for the vast majority of cases. This isn't extremely important but maybe another opinion would help clarify.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems fine to me. Still readable.

@roji roji force-pushed the ScaffoldOnUpdate branch from 4f0f24b to aa39b32 Compare October 28, 2019 17:08
@ajcvickers
Copy link
Contributor

/cc @Pilchie

@roji roji merged commit f28c7db into release/3.1 Oct 29, 2019
@roji roji deleted the ScaffoldOnUpdate branch October 29, 2019 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scaffolding column with ValueGenerated.OnUpdate generates invalid C# code
3 participants