dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.76k stars 3.18k forks source link

Scaffold-DbContext can name namespace wrong if phrase with space is used on OutputDir #24607

Open hahyes opened 3 years ago

hahyes commented 3 years ago

Scaffold-DbContext is naming namespace wrong if -OutputDir is filled with phrase containing space.

For example: Example Folder OutputDir makes namespace ConsoleApp.Example Folder, what is wrong for naming namespaces and can't be builded. It should be ConsoleApp.Example_Folder.

Code

I made command through PMC like this:

Scaffold-DbContext "connectionString" MySql.EntityFrameworkCore -Tables some_tables_here -OutputDir "Example Folder" -Force

Command is working, but output classes and context have wrong namespace.

Include provider and version information

EF Core version: 5.0 Database provider: MySql.EntityFrameworkCore Target framework: NET 5.0 Operating system: IDE: Visual Studio 2019 16.9.3

Psypher9 commented 3 years ago

Is this open for contributions?

Psypher9 commented 3 years ago

@bricelam , so I've been looking at this a little bit and it looks like the private function that gets the subnamespace inside of the EFCore.Design.DatabaseOperations.ScaffoldContext needs to have a .Replace(' ', '_') added to this ternary before the string.Join('.',...) is evaluated.

I also happened to notice that the GetNamespaceFromOutputPath method here and SubnamespaceFromOutputPath method here are the exact same inside of DbContextOperations.Optimize.

I've been playing around with this in my own branch and I propose making a service to handle generating namespaces based on file paths and

  1. have both DbContextOperations.Optimize and DatabaseOperations.ScaffoldContext use that service
  2. have that service use the .Replace(' ', '_') to handle spaces in output paths.

Another reason I propose making this a service is that, when I was looking to add tests there wasn't really a good way to make tests for this particular thing for either use case (at least to my knowledge). I've added tests to support my work so far, but is this something you guys really want and if so, do you really want to replaces spaces with _ or just remove them entirely?

Thanks!

bricelam commented 3 years ago

Hmm my initial thoughts on this were to use CSharpUtilities.GenerateCSharpIdentifier when generating the namespace in C#. This is because different languages have different requirements for what constitutes a valid identifier.

But MSBuild also handles it similarly to the way you've proposed, so maybe it's ok...

Happy to consider any efforts to avoid duplicate code.

Psypher9 commented 3 years ago

Ah okay, I'll check out the GenerateCSharpIdentifier method. I was unaware of that utility, so I'll how I can use it in this case. Hopefully, we won't need a new service!

If MSBuild handles the namespace generation in the same way, I'll keep moving forward with that if you guys are alright with that. I'm happy to handle it anyway that fits with what you all want to do.

bricelam commented 3 years ago

Yes, I'm OK with this approach. It would also be nice to do it for Migration class names.

mees- commented 3 years ago

I encountered the same issue when using a dash "-" in the output directory name. All the namespaces get invalid names and the classes wont build.