-
Notifications
You must be signed in to change notification settings - Fork 627
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
AdminController: adding unit tests #610
AdminController: adding unit tests #610
Conversation
pinging @mheggeseth @MisterJames for a quick review before we move forward. Thanks! |
return View("Error"); | ||
} | ||
|
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 should add this to the style guide, but we do prefer having the braces around if and else clauses, even for single statement clauses.
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.
@BillWagner crap. I've been coding all along to take the curly braces out of single line statement clauses. Going forward I'll adhere to that approach
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 feel bad because we should have documented that expectation. We'll get there.
Left a few comments. And create this issue: HTBox/htbox.github.io#4 Once the comments are addressed, and the merge conflicts resolved, this is ready to merge. |
@BillWagner, how do I go about handling these merge conflicts? The regular steps for rebasing from master? |
@mgmccarthy Yes, that's right. Rebase from master (which will show the merge conflicts) and resolve them. |
…neralSettings to IOptions<SampleDataSettings> IOptions<GeneralSettings>intead of SampleDataSettings and GeneralSettings so unit test to not have to provide a mocked instance of each type to AdminController's constructor when these two setting are only used one each in the controller code.
pinging @BillWagner. All merge conflicts fixed. This is ready to go. |
fixes #609