-
Notifications
You must be signed in to change notification settings - Fork 250
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
chore: various channel follow-ups #2796
Conversation
1cbb6ca
to
649d21a
Compare
1acbd42
to
47249a0
Compare
649d21a
to
6a76a44
Compare
@@ -87,3 +87,8 @@ internal static string EvaluationScript(string content, string path) | |||
throw new ArgumentException("Either path or content property must be present"); | |||
} | |||
} | |||
|
|||
internal class EvaluateArgumentGuidElement |
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.
Doesn't C# style gives preference to file per class?
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 do this already in other parts in order to keep the amount of source code files more clear.
property.SetValue(objResult, ToExpectedType(kv.Value, property.PropertyType, visited)); | ||
} | ||
var property = Array.Find(t.GetProperties(), prop => string.Equals(prop.Name, kv.Key, StringComparison.OrdinalIgnoreCase)); | ||
property?.SetValue(objResult, ToExpectedType(kv.Value, property.PropertyType, visited)); |
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.
What is the minimal .Net version that supports ?.
operator, are we confident that we'll not need to build with it?
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.
.NET takes care of that via the TFM, we can use any newer C# language feature how we like.
Gets rebased on #2795 once landed.