-
Notifications
You must be signed in to change notification settings - Fork 151
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
Support TargetDefaultWrapper and TargetDefaultParameters #500
Conversation
Codecov Report
@@ Coverage Diff @@
## master #500 +/- ##
==========================================
- Coverage 80.96% 80.59% -0.38%
==========================================
Files 16 16
Lines 1345 1355 +10
Branches 234 230 -4
==========================================
+ Hits 1089 1092 +3
- Misses 172 175 +3
- Partials 84 88 +4
Continue to review full report at Codecov.
|
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.
Looks good, 1 general question. Not sure if we need to change it.
} | ||
} | ||
} | ||
|
||
private bool AlreadyReadChild(IConfigurationSection child, IConfigurationSection variables, IConfigurationSection defaultWrapper, IConfigurationSection defaultTargetParameters) | ||
private bool AlreadyReadChild(IConfigurationSection child, IConfigurationSection variables, IConfigurationSection targetDefaultWrapper, IConfigurationSection targetDefaultParameters) |
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.
Isn't defaultTargetParameters more in line with:
✔️ DO choose easily readable identifier names.
For example, a property named HorizontalAlignment is more English-readable than AlignmentHorizontal.
From https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/general-naming-conventions
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.
Well I like that first word specifies the area of functionality. And "default" is very generic and gives no information. Just like I prefer FilterDefaultAction
over DefaultFilterAction
.
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.
DefaultFilterAction is also more in line with english and the microsoft guideline.
Codecov Report
@@ Coverage Diff @@
## master #500 +/- ##
==========================================
- Coverage 80.96% 80.59% -0.38%
==========================================
Files 16 16
Lines 1345 1355 +10
Branches 234 230 -4
==========================================
+ Hits 1089 1092 +3
- Misses 172 175 +3
- Partials 84 88 +4
Continue to review full report at Codecov.
|
Resolves #499