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

Commands: Inject IHostingEnvironment, IApplicationEnvironment, etc. into Startup #4733

Merged
merged 5 commits into from
Mar 11, 2016
Merged

Conversation

bricelam
Copy link
Contributor

@bricelam bricelam commented Mar 8, 2016

Other small changes

@NTaylorMullen This is the PR you're looking for... 👋

Part of #3925

@@ -37,7 +37,7 @@ public static void Configure([NotNull] CommandLineApplication command)
"-o|--output-dir <path>",
"Directory of the project where the classes should be output. If omitted, the top-level project directory is used.");
var schemas = command.Option(
"-s|--schema <schema>",
"--schema <schema>",
Copy link
Contributor

Choose a reason for hiding this comment

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

@bricelam This is OK for now if you need to get this in. But I'd prefer to come up with a new short-form for --schema. It's odd that --table still has one but --schema does not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the fix we decided on in triage. You can create a new issue if you think it's common enough to deserve a short name. Personally, I think we should remove even more short names.

Copy link
Contributor

Choose a reason for hiding this comment

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

If table is going to be more commonly used than schema then I think having a short name for table makes sense and I don't think we need symmetry between table and schema. Otherwise I think we should reassess.

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I think we should remove even more short names.

Agreed, I think we've gone a little overboard with them. To me they are most useful when you type by hand a lot (-verbose etc.). I imagine things like table/schema/etc. will generally be stored in a script etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think users will find the lack of symmetry odd. But I'm OK if we wait and see. It will be easy to add some abbreviation back in if we find it's wanted. Since we have no exclusion syntax, I do expect that the average number of -t|--tables on the command line will exceed the number of --schemas, sometimes significantly.

@NTaylorMullen
Copy link
Contributor

What's the override route for the case when you have custom services (from Program.Main) being injected into your Startup.cs? It still looks like Startup.cs is constructed every time no matter what without fallback to IDbContextFactory.

@bricelam
Copy link
Contributor Author

bricelam commented Mar 9, 2016

@NTaylorMullen We'll sort that out as part of #4546. Skipping Startup will be done as part of #4710. But I think this should unblock 80% of scenarios (including yours).

@rowanmiller
Copy link
Contributor

:shipit:

@bricelam bricelam merged commit 7a76320 into dotnet:dev Mar 11, 2016
@bricelam bricelam deleted the host branch March 11, 2016 20:11
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.

6 participants