Added required parameters. #370
Conversation
blackbaud-brandonhare
requested review from
Blackbaud-BobbyEarl,
Blackbaud-PaulCrowder and
Blackbaud-SteveBrush
…kyux-builder into feature-required-param
…hare:blackbaud/skyux-builder into feature-required-param
Codecov Report
@@ Coverage Diff @@
## master #370 +/- ##
==========================================
+ Coverage 99.24% 99.24% +<.01%
==========================================
Files 73 73
Lines 1718 1729 +11
Branches 273 276 +3
==========================================
+ Hits 1705 1716 +11
Misses 13 13
Continue to review full report at Codecov.
|
runtime/params.ts
Outdated
| return true; | ||
| } | ||
|
|
||
| return this.requiredParams.some((param: string) => { |
There was a problem hiding this comment.
Based on my understanding of some(), this would return true if there's at least one required param with a value. This would return a false positive in the case where there are more than one required parameters but at least one required parameter doesn't have a value. I see there's not a unit test that tests this scenario, so I'd like to see that test written as well as any necessary changes to this code.
There was a problem hiding this comment.
Good catch. Swapped that to every, which is actually what we want. I've also added a test for that condition.
* Added required parameters. * Adding types. * Added type. * Fixed formatting. * fixed formatting. * Fixed space. * Fixed formatting. * Changes based on feedback. * Fixed test. * Fixed check for required params.
I'll update the tests, but I wanted to get some feedback on the implementation first. I'll also fix the formatting (I'm using prettier, thought I turned it off).