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

Fixed up dotsettings file, changed to some c# 7 features #18

Merged
merged 1 commit into from
Oct 6, 2017

Conversation

JoeStead
Copy link
Collaborator

@JoeStead JoeStead commented Oct 4, 2017

The dot settings file was still using C# 5.0 for some reason, I removed this and:

  • Used inline functions for the request delegate, so no need to cast it any more
  • Made the methods in Botwin module protected, there was no need for them to be public
  • Removed some unused using statements

@@ -22,7 +22,7 @@ private IValidator FindValidator(Type type)
var fullType = CreateValidatorType(type);

var available = this.validators
.Where(validator => fullType.GetTypeInfo().IsAssignableFrom(validator.GetType()))
.Where(validator => fullType.GetTypeInfo().IsInstanceOfType(validator))
Copy link
Member

Choose a reason for hiding this comment

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

Did you run tests for this. I saw Rider suggest this but I'm not sure it does what I want

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Did I run tests before sending a PR... it's like you don't even know me. (Yes, the tests pass)

Copy link
Member

Choose a reason for hiding this comment

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

I need to get a CI build setup on this repo. That's your next PR 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

AppVeyor -> New Project, right?

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
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately for you, it will have to be the repo admin that does it. Unlucky.

@jchannon
Copy link
Member

jchannon commented Oct 4, 2017 via email

@JoeStead JoeStead force-pushed the language-moderniser branch from 04082ec to f6bd2a3 Compare October 6, 2017 09:00
@JoeStead JoeStead force-pushed the language-moderniser branch from f6bd2a3 to 12517b4 Compare October 6, 2017 09:02
RequestDelegate requestDelegate = httpContext =>
handler(httpContext.Request, httpContext.Response, httpContext.GetRouteData());
this.Get(path, requestDelegate);
Task RequestDelegate(HttpContext httpContext) => handler(httpContext.Request, httpContext.Response, httpContext.GetRouteData());
Copy link
Member

Choose a reason for hiding this comment

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

What is this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

An inline function. C# 7 thing. Makes working with functions and the like way simpler

Copy link
Member

Choose a reason for hiding this comment

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

Adds no value here IMO

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There was a reason I did this, if you asked me 2 days ago I would have told you

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jchannon jchannon merged commit 895635a into CarterCommunity:master Oct 6, 2017
@JoeStead JoeStead deleted the language-moderniser branch October 6, 2017 12:53
@jchannon jchannon added this to the 2.0.0 milestone Oct 12, 2017
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.

2 participants