-
Notifications
You must be signed in to change notification settings - Fork 25.3k
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
Rc1-Rc2 #1253
Rc1-Rc2 #1253
Conversation
@JunTaoLuo @DamianEdwards @Rick-Anderson Please review |
@@ -17,7 +17,7 @@ For information regarding a complete list of breaking changes in RC2 for ASP.NET | |||
Creating your web application host | |||
---------------------------------- | |||
|
|||
Since ASP.NET Core apps are just console apps, you must define an entry point for your application in ``Program.Main`` that sets up a web host, then tells it to start listening. Below is an example of the startup code for the built-in `Web Application` template in Visual Studio. | |||
Since ASP.NET Core apps are just console apps, you must define an entry point for your application in ``Program.Main()`` that sets up a web host, then tells it to start listening. Below is an example of the startup code for the built-in `Web Application` template in Visual Studio. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then tell it to start listening
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since "the entry point" is the subject... "tells" is the correct grammar. To verify, use the standard grammar technique of removing the superfluous verbiage, "for your application in Program.Main()
that sets up a web host" and read the sentence, which is then "the entry point tells it to start listening".
(See the Strunk & White manual for clarification). I can reword for two sentences though if it sounds more appealing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read the sentence with you being the subject. As in you must define ... then you tell it to start listening. It's awkward phrasing and splitting it into two sentence is much better.
See new DISQUS comments. Also, can you put the article title in the PR next time? |
ASPNET_APP, ASPNET_APPLICATIONNAME, Hosting:App ASPNETCORE_APPLICATIONNAME | ||
ASPNET_STARTUPASSEMBLY ASPNETCORE_STARTUPASSEMBLY | ||
ASPNET_DETAILEDERRORS, Hosting:DetailedErrors ASPNETCORE_DETAILEDERRORS | ||
ASPNET_ENVIRONMENT, ASPNET_ENV, Hosting:Environment ASPNETCORE_ENVIRONMENT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still support ASPNET_ENV
, Hosting:Environment
in RC2 but the user will see a message indicating these values are deprecated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JunTaoLuo No need to mention that since the legacy env vars will go away.
@danroth27 these changes looks accurate to me and we should update the document soon. If there are further changes we want to make (like re-ordering of sections), we should do that now or in a follow up PR. |
@danroth27 need a tree rat |
@@ -14,10 +14,53 @@ ASP.NET 5 RC1 apps were based on the .NET Execution Environment (DNX) and made u | |||
|
|||
For information regarding a complete list of breaking changes in RC2 for ASP.NET Core, see the `ASP.NET Core Announcements page <https://github.com/aspnet/announcements/issues?q=is%3Aopen+is%3Aissue+milestone%3A1.0.0-rc2>`_ | |||
|
|||
Changes in MVC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Starting with MVC feels weird to me since it is at the top of the stack. Can we move this below "Namespace and package ID changes"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Starting at line 159:
This seems to be going through changes that were made between RC1 and RC2. I think both paragraphs can be removed. Specifying a configuration instance for the web host is covered at the end of the "Creating your web application host" section. |
@@ -136,7 +193,7 @@ HttpPlatformModule | |||
|
|||
``Microsoft.AspNetCore.IISPlatformHandler`` is now ``Microsoft.AspNetCore.Server.IISIntegration``. | |||
|
|||
HttpPlatformModule was replaced by ASP.NET Core Module. The web.config created by the Publish IIS tool now configures IIS to use ASP.NET Core Module instead of HttpPlatformHandler to reverse-proxy requests to Kestrel. | |||
HttpPlatformModule was replaced by ASP.NET Core Module. The ``web.config`` created by the Publish IIS tool now configures IIS to use ASP.NET Core Module instead of HttpPlatformHandler to reverse-proxy requests to Kestrel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HttpPlatformModule -> HTTP Platform Handler. Fix throughout the doc.
I would also remove the subsection headings from the "Working with IIS" section and just cover the new ASP.NET Core Module. Link to "https://docs.asp.net/en/latest/publishing/iis.html" for details about publishing to IIS in RC2 instead of covering the details of the publish-iis tool here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignore the above comment.
Here are the top level sections I think we should have:
Other changes:
Rework the "Hosting configuration" section as follows:
In the "Creating your web application host" section add the following sentence below the code snippet: "The application base path is now called the content root." |
⌚ |
cust writes |
|
||
IHostingEnvironment changes | ||
--------------------------- | ||
-application |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't rendering as a list correctly. You have to have a space after the -
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
⌚ |
|
||
WebHostBuilder API updates | ||
-------------------------- | ||
"The default server URL and port are ``localhost:5000``. You can find more information about Garbage Collection configuration at: https://github.com/aspnet/Announcements/issues/175 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove leading quote
A couple more minor things, but once those are fixed |
d1bfb2a
to
fd55156
Compare
#1227