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

Migrations Design: Invalid 'using' statements for default namespace #2467

Closed
natemcmaster opened this issue Jun 24, 2015 · 16 comments
Closed

Migrations Design: Invalid 'using' statements for default namespace #2467

natemcmaster opened this issue Jun 24, 2015 · 16 comments

Comments

@natemcmaster
Copy link
Contributor

@natemcmaster natemcmaster commented Jun 24, 2015

Invalid using ; added to generated migration file because this simple project has no default namespace.

Setup https://gist.github.com/natemcmaster/90e2388c0ee1d29f440a.

Using dnx 1.0.0-beta6-12114 , EF 7.0.0-beta6-13584

dnx . ef migration add Initial

produces 20150624223653_Initial.Designer.cs which includes this at the top of the file:

using System;
using Microsoft.Data.Entity;
using Microsoft.Data.Entity.Metadata;
using Microsoft.Data.Entity.Relational.Migrations.Infrastructure;
using ;

The same using block appears in ModelSnapshot.cs

@natemcmaster natemcmaster added cross-plat and removed cross-plat labels Jun 24, 2015
@bricelam

This comment has been minimized.

Copy link
Contributor

@bricelam bricelam commented Jun 26, 2015

Related to #2261

@softmarshmallow

This comment has been minimized.

Copy link

@softmarshmallow softmarshmallow commented Jul 23, 2017

this issue still occurs, and all entity under Proj.Models....

@Rick-Anderson

This comment has been minimized.

Copy link

@Rick-Anderson Rick-Anderson commented Aug 18, 2017

FWIW, more customers will be hitting this now that we're creating more CLI docs. See aspnet/Scaffolding#599

@ajcvickers

This comment has been minimized.

Copy link
Member

@ajcvickers ajcvickers commented Aug 18, 2017

@Rick-Anderson Why do we have sample projects with no default namespace?

@Rick-Anderson

This comment has been minimized.

Copy link

@Rick-Anderson Rick-Anderson commented Aug 18, 2017

@ajcvickers maybe this is rare. When you follow the instructions at aspnet/Scaffolding#599, you get no namespace. The project has a namespace, not the DC.
It's not clear the CLI requires you to add a namespace for the DC.

@ajcvickers

This comment has been minimized.

Copy link
Member

@ajcvickers ajcvickers commented Aug 18, 2017

@Rick-Anderson It may not be required, but it's certainly best practice. If we have some guidance or tooling that is by default creating classes not in any namespace, then I think the real thing to address is that guidance and tooling. (Think about what VS does--every project you create or class you add to it is in a namespace by default; it's idiomatic to .NET.)

@Rick-Anderson

This comment has been minimized.

Copy link

@Rick-Anderson Rick-Anderson commented Aug 18, 2017

@ajcvickers does the bug belong to .NET CLI for not warning on -dc MovieContext in the following command?

dotnet aspnet-codegenerator razorpage -m Movie -dc MovieContext -udl -outDir Pages\Movies --referenceScriptLibraries

@ajcvickers

This comment has been minimized.

Copy link
Member

@ajcvickers ajcvickers commented Aug 18, 2017

@Rick-Anderson I don't know enough about how the templating works to know what part should be ensuring a namespace. Maybe ask whoever authored the templates?

@Rick-Anderson

This comment has been minimized.

Copy link

@Rick-Anderson Rick-Anderson commented Aug 18, 2017

@ajcvickers But it is generating invalid code. You're not required to have a NS. It seems like it would be better to fix the producer of bad generated code than fixing the inputs.

@ajcvickers

This comment has been minimized.

Copy link
Member

@ajcvickers ajcvickers commented Aug 18, 2017

@Rick-Anderson We didn't close this--we are intending to fix it. We will re-triage the priority. I still believe the bigger issue is that something out of Microsoft if generating a class without a namespace. That's something we should just not do.

@ajcvickers ajcvickers removed this from the Backlog milestone Aug 18, 2017
@Rick-Anderson

This comment has been minimized.

Copy link

@Rick-Anderson Rick-Anderson commented Aug 18, 2017

@ajcvickers I'm guessing this is low priority FWIW.
I sometimes create VS projects and remove the namespace - just to cut down on noise/indenting on rendered code. Even if the scaffolder forces a namespace, nothing stops the user from removing the namespace. I'll shut up now.

@ajcvickers ajcvickers added this to the 2.1.0 milestone Aug 23, 2017
@bricelam bricelam changed the title Invalid 'using' statements generated by migration scaffolding (default namespace) Migrations Design: Invalid 'using' statements for default namespace Sep 8, 2017
@julielerman

This comment has been minimized.

Copy link

@julielerman julielerman commented Oct 24, 2017

Would love to see this problem go away. Yes, every class should have a namespace but this is a rough punishment. ;)

@smitpatel

This comment has been minimized.

Copy link
Member

@smitpatel smitpatel commented Oct 24, 2017

https://github.com/aspnet/EntityFrameworkCore/blob/ed629d65089bc7b1bbd6853c335e541df5a5ae7e/src/EFCore.Design/Migrations/Design/CSharpMigrationsGenerator.cs#L158

Where empty string is being injected. Just need a check there before printing out using statement for it.

smitpatel added a commit that referenced this issue Nov 8, 2017
Don't generate empty namespace if context is not in namespce

Resolves #10225
Resolves #2467
@smitpatel smitpatel assigned smitpatel and unassigned bricelam Nov 8, 2017
smitpatel added a commit that referenced this issue Nov 9, 2017
Don't generate empty namespace if context is not in namespce

Resolves #10225
Resolves #2467
@BennyM

This comment has been minimized.

Copy link

@BennyM BennyM commented Nov 17, 2017

Awesome, I was just about to clone the repo and fix this :)

@bricelam

This comment has been minimized.

Copy link
Contributor

@bricelam bricelam commented Nov 17, 2017

@BennyM I love the enthusiasm! Feel free to check out some of our other up-for-grabs issues if you're still interested in submitting a PR. 😉

@BlinkSun

This comment has been minimized.

Copy link

@BlinkSun BlinkSun commented Nov 28, 2017

oh didnt see that #2467 !!! Thank you !!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
10 participants
You can’t perform that action at this time.