Default to using Humanizer.Core as reverse engineeer pluralizer #20577

ErikEJ · 2020-04-09T13:00:02Z

Fix test localization bug

fixes #11160

ErikEJ · 2020-04-09T14:24:07Z

How do I investigate the test failures? (all tests passed on my Windows machine)

smitpatel · 2020-04-09T19:49:34Z

Seems like intermittent issue. I restarted runs.

ErikEJ · 2020-04-10T08:25:03Z

@smitpatel Thanks, tests still fail - how do I as a contributor view the logs?

smitpatel · 2020-04-10T21:34:44Z

Click on details beside failed build. (takes you to github checks page)
Click on "View more details on Azure Pipelines" at the end of the page.
That will take you to azure pipeline builds. You can click on build number to reach the build page for particular PR. There there is "Tests" tab to see test results. Also click on published will give you build artifacts, (logs generated)

ErikEJ · 2020-04-11T06:14:31Z

@smitpatel Thanks a lot "You can click on build number to reach the build page for particular PR. " was the missing link! The "View more details" link was too deep 😄

ajcvickers · 2020-04-16T18:48:10Z

src/EFCore.Design/EFCore.Design.csproj

@@ -26,6 +26,7 @@
  </ItemGroup>

  <ItemGroup>
+    <PackageReference Include="Humanizer.Core" Version="$(HumanizerCorePackageVersion)" />


@dotnet/efcore We should discuss whether or not we want to take a direct dependency here.

may be privateassets all would solve our concerns.

Maybe, tried that, but then the tests failed

Because the dependency wouldn't flow into test project either. To clear up confusion, if you use privateassets all and use the tool in user application, does it pluralize/singularize correctly? We can hack our way for tests if functionality for user works.

I have no idea how to test that, but if someone can tell me how, I can give it a try.

@smitpatel Sorry, I cannot figure out how to do that - can you provide more detailed steps.
I try to run from with VS with F5, and set the following debug parameters in project properties/Debug:
dbcontext scaffold "Data Source=(localdb)\mssqllocaldb;Intital Catalog=Northwind;Integrated Security=true" Microsoft.EntityFrameworkCore.SqlServer -o C:\temp\test\ef

But get: Missing required option '--assembly'.

I can run: dbcontext scaffold --help and get a useful help message

I have done such testing on command line only. No trust for VS. Alternatively, you should be able to build package with these changes and use the custom build package in a sample app to install EFCore.Tools and use PMC commands to test out in VS.
I will investigate on Monday.

I wish there was a simple way to do this, that sounds exceedingly complicated @bricelam ??

It is not important for me to test in VS, but the command line throws same error!

FYI: I added "privateAssets all" and added an explicit reference from the Desing.Test project - that worked.

ErikEJ · 2020-04-19T12:05:24Z

How do I integration test this?

src/EFCore.Design/Design/DesignTimeServiceCollectionExtensions.cs

smitpatel · 2020-04-20T18:00:17Z

@ErikEJ - You can add integration tests in CSharpEntityTypeGeneratorTest.

ajcvickers · 2020-04-20T21:04:36Z

@bricelam I'm pretty sure we decided that it was okay to have pluralization on by default for reverse engineering, right? That is, we won't consider it a breaking change if the pluralization gets updated and we then start scaffolding with different names.

ajcvickers · 2020-04-20T21:07:11Z

@Pilchie We want to bring in https://www.nuget.org/packages/Humanizer.Core/ as a third-party dependency for Microsoft.EntityFrameworkCore.Design. What steps do we need to take?

bricelam · 2020-04-24T22:15:07Z

We decided that it was okay to have pluralization on by default for reverse engineering, right?

Let's discuss in a design meeting. I feel like it's ok so long as we have an option (e.g. --no-pluralize) to go back to previous behavior.

bricelam

Looking good! Sounds like we have a couple things to discuss in the next design meeting.

src/EFCore.Design/EFCore.Design.csproj

test/EFCore.Design.Tests/EFCore.Design.Tests.csproj

src/EFCore.Design/Design/DesignTimeServiceCollectionExtensions.cs

bricelam

Changes look good.

Things to discuss as a team:

Should we remove NullPluralizer
Should we pluralize by default?
- If so, do we need an option to disable pluralization?

bricelam · 2020-05-13T19:52:12Z

Discussed as a team. Decisions:

We should continue pluralizing by default
Need to add --no-pluralize option
- Let's do this before merging so we don't disrupt daily builds or previews
- We can remove NullPluralizer since --no-pluralize will have the same behavior

Other follow-up questions:

Is Humanizer culture sensitive?
- If so, will users want an option to specify the culture? (But YAGNI for now)

ErikEJ · 2020-05-14T11:18:17Z

--no-pluralize option

Any proposal for implementation? How would you pass that all the way through - to the Pluralizer?

Let's do this before merging so we don't disrupt daily builds or previews

How would that prevent disruption, since pluralization is now on by default, and disabling it is an opt-in?

ajcvickers · 2020-05-14T15:41:26Z

@ErikEJ We just want to avoid having people not be able to use the preview because they can't switch the behavior back to the way they want it without messing with services.

ErikEJ · 2020-05-14T15:45:26Z

Makes sense. How do you propose floating the option? How to implement "opt out" ?

ajcvickers · 2020-05-14T15:56:22Z

@ErikEJ No idea. :-) I'd have to read the code. @bricelam might have ideas already.

ErikEJ · 2020-05-14T15:57:27Z

Fine, will await some proposals from @bricelam

bricelam · 2020-05-14T21:57:46Z

Updated:

Rebased on master and squashed
Implemented --no-pluralize.

src/EFCore.Design/Scaffolding/Internal/RelationalScaffoldingModelFactory.cs

src/EFCore.Design/Scaffolding/ModelReverseEngineerOptions.cs

ErikEJ · 2020-05-15T12:19:36Z

@bricelam Thanks, Neo!

Fix test localization bug fixes dotnet#11160

ErikEJ requested a review from dougbu as a code owner April 9, 2020 13:00

ajcvickers reviewed Apr 16, 2020

View reviewed changes

ajcvickers assigned bricelam Apr 17, 2020

ErikEJ force-pushed the issue-11160 branch from 08cba90 to 36bf975 Compare April 19, 2020 12:04

smitpatel reviewed Apr 20, 2020

View reviewed changes

src/EFCore.Design/Design/DesignTimeServiceCollectionExtensions.cs Show resolved Hide resolved

bricelam reviewed Apr 24, 2020

View reviewed changes

src/EFCore.Design/EFCore.Design.csproj Outdated Show resolved Hide resolved

test/EFCore.Design.Tests/EFCore.Design.Tests.csproj Outdated Show resolved Hide resolved

src/EFCore.Design/Design/DesignTimeServiceCollectionExtensions.cs Show resolved Hide resolved

bricelam added the needs-design label Apr 24, 2020

bricelam approved these changes Apr 30, 2020

View reviewed changes

bricelam added waiting-for-response and removed needs-design labels May 13, 2020

bricelam force-pushed the issue-11160 branch from b762b6a to 0c3b907 Compare May 14, 2020 21:56

smitpatel reviewed May 14, 2020

View reviewed changes

src/EFCore.Design/Scaffolding/Internal/RelationalScaffoldingModelFactory.cs Outdated Show resolved Hide resolved

smitpatel reviewed May 14, 2020

View reviewed changes

src/EFCore.Design/Scaffolding/ModelReverseEngineerOptions.cs Outdated Show resolved Hide resolved

bricelam removed the waiting-for-response label May 26, 2020

bricelam force-pushed the issue-11160 branch 2 times, most recently from 374605b to f43c845 Compare June 16, 2020 15:53

ErikEJ and others added 2 commits June 16, 2020 14:41

Default to using Humanizer.Core as reverse engineeer pluralizer

3f1b2f7

Fix test localization bug fixes dotnet#11160

Tools: Add --no-pluralize option

fde7352

bricelam force-pushed the issue-11160 branch from f43c845 to fde7352 Compare June 16, 2020 21:42

bricelam merged commit fde7352 into dotnet:master Jun 16, 2020

ajcvickers mentioned this pull request Jun 18, 2020

Entity Framework Weekly Status Updates (2020) #19549

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default to using Humanizer.Core as reverse engineeer pluralizer #20577

Default to using Humanizer.Core as reverse engineeer pluralizer #20577

ErikEJ commented Apr 9, 2020

ErikEJ commented Apr 9, 2020

smitpatel commented Apr 9, 2020

ErikEJ commented Apr 10, 2020

smitpatel commented Apr 10, 2020

ErikEJ commented Apr 11, 2020

ajcvickers Apr 16, 2020

smitpatel Apr 16, 2020

ErikEJ Apr 17, 2020

smitpatel Apr 17, 2020

ErikEJ Apr 17, 2020

ErikEJ Apr 18, 2020

smitpatel Apr 18, 2020

ErikEJ Apr 19, 2020

ErikEJ Apr 19, 2020

ErikEJ Apr 19, 2020

ErikEJ commented Apr 19, 2020

smitpatel commented Apr 20, 2020

ajcvickers commented Apr 20, 2020

ajcvickers commented Apr 20, 2020

bricelam commented Apr 24, 2020 •

edited

Loading

bricelam left a comment

bricelam left a comment

bricelam commented May 13, 2020 •

edited

Loading

ErikEJ commented May 14, 2020 •

edited

Loading

ajcvickers commented May 14, 2020

ErikEJ commented May 14, 2020

ajcvickers commented May 14, 2020

ErikEJ commented May 14, 2020

bricelam commented May 14, 2020

ErikEJ commented May 15, 2020

Default to using Humanizer.Core as reverse engineeer pluralizer #20577

Default to using Humanizer.Core as reverse engineeer pluralizer #20577

Conversation

ErikEJ commented Apr 9, 2020

ErikEJ commented Apr 9, 2020

smitpatel commented Apr 9, 2020

ErikEJ commented Apr 10, 2020

smitpatel commented Apr 10, 2020

ErikEJ commented Apr 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErikEJ commented Apr 19, 2020

smitpatel commented Apr 20, 2020

ajcvickers commented Apr 20, 2020

ajcvickers commented Apr 20, 2020

bricelam commented Apr 24, 2020 • edited Loading

bricelam left a comment

Choose a reason for hiding this comment

bricelam left a comment

Choose a reason for hiding this comment

bricelam commented May 13, 2020 • edited Loading

ErikEJ commented May 14, 2020 • edited Loading

ajcvickers commented May 14, 2020

ErikEJ commented May 14, 2020

ajcvickers commented May 14, 2020

ErikEJ commented May 14, 2020

bricelam commented May 14, 2020

ErikEJ commented May 15, 2020

bricelam commented Apr 24, 2020 •

edited

Loading

bricelam commented May 13, 2020 •

edited

Loading

ErikEJ commented May 14, 2020 •

edited

Loading