Skip to content
This repository was archived by the owner on Mar 19, 2019. It is now read-only.

Merge and rename packages and components to HttpSys* #286

Merged
merged 6 commits into from
Jan 3, 2017
Merged

Conversation

Tratcher
Copy link
Member

#283

Stage 1: merge packages, namespaces, tests, initial renames, reduced object visibility

Future work:

  • internal refactoring to reduce object model duplication across layers
  • perf optimizations possible via deeper object model integration

@Tratcher Tratcher added this to the 2.0.0 milestone Dec 16, 2016
@Tratcher Tratcher self-assigned this Dec 16, 2016
{
return hostBuilder.ConfigureServices(services => {
services.AddTransient<IConfigureOptions<WebListenerOptions>, WebListenerOptionsSetup>();
Copy link
Member

Choose a reason for hiding this comment

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

Why isn't this his required?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was used to set up a logger, which can now be passed directly from DI.

@@ -23,6 +23,7 @@
},
"net451": {
"frameworkAssemblies": {
"System.DirectoryServices": "",
Copy link
Member

Choose a reason for hiding this comment

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

???

Copy link
Member Author

Choose a reason for hiding this comment

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

The NTLM tests only work on domain.

Copy link
Member

Choose a reason for hiding this comment

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

What APIs are you using that you weren't using before (or did I just miss that).

Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't new, it's just merged from the other test project.

{
internal class ResponseStream : Stream
internal class ResponseBody : Stream
Copy link
Member

Choose a reason for hiding this comment

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

Just renaming for fun?

Copy link
Member Author

Choose a reason for hiding this comment

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

There were two ResponseSteam classes. I may be able to de-duplicate later.

Copy link
Contributor

@JunTaoLuo JunTaoLuo left a comment

Choose a reason for hiding this comment

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

Why are the travis builds broken? Do more tests need to be skipped?

@@ -13,6 +13,7 @@ internal static class Constants
internal const string Close = "close";
internal const string Zero = "0";
internal const string SchemeDelimiter = "://";
internal const int Status101SwitchingProtocols = 101;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be moved to StatusCodes in HttpAbstractions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. Can you add that this week?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

aspnet/HttpAbstractions#748, any other status codes we want to add?

Copy link
Contributor

@JunTaoLuo JunTaoLuo left a comment

Choose a reason for hiding this comment

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

Other than deciding what to do for that one status code and fixing the tests.

@Tratcher
Copy link
Member Author

Tratcher commented Jan 3, 2017

@JunTaoLuo Your commit regressed the tests xplat 3b42433. You forgot to make them conditional like all of the others.

@JunTaoLuo
Copy link
Contributor

@Tratcher ah I see. Didn't realize those need to be updated.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants