Bugfix/vmo 2010/numeric block max characters validation #30
Conversation
bulatgab
commented
Jan 20, 2021
•
bulatgab
commented
- Replaced ValidationException with PromptValidationException in all prompt implementations because that is what BasePrompt is catching in its 'set value()'
- Fixed BasePrompt not resetting this.error after a valid value was entered
- Fixed SelectManyPrompt demanding non-empty selections[] when choices[] is empty.
- Added null and undefined handling in all prompt.validate() implementations because IPromptConfig.value can be null or undefined
- Removed BasePrompt.error as redundant (we assign the value anyway, even if the value is not valid), it's probably better to use isValid() instead
- Replaced prompt.validate() with validateOrThrow() that has a different signature (returns void instead of boolean) which removes the ambiguity of how to use it. The validate() remains in the BasePrompt if the user of the library might need it, it returns boolean and doesn't throw
- Fixed validateOrThrow implementations' return and throw lines
- Cleaned up BasePrompt: removed try-catch from 'set value' as we assign the value even if there is an error thrown, changed the fulfill() so that it throws an error if the value is invalid which means the flow runner doesn't proceed/run with an invalid value anymore
- Replaced InvalidChoiceException thrown in SelectManyPrompt.validateOrThrow() with PromptValidationException, so now we expect only that type of error from all validateOrThrow() implementations which is supposed to make things simpler and less error-prone
- Updated tests and readme file.
…tionException with PromptValidationException in all prompt implementations because that is what BasePrompt is catching in its 'set value()'. Fixed BasePrompt not resetting this.error after a valid value was entered. Fixed SelectManyPrompt demanding non-empty selections[] when choices[] is empty.
…a suggestion. Added null and undefined handling in all prompt.validate() implementations because IPromptConfig.value can be null or undefined. Removed BasePrompt.error as redundant (we assign the value anyway, even if the value is not valid), maybe it's better to use isValid() instead?
… validate() with validateOrThrow() that has a different signature (returns void instead of boolean) which removes the ambiguity of how to use it. The validate() remains in the BasePrompt if the user of the library might need it, it returns boolean and doesn't throw. Fixed validateOrThrow implementations' return and throw lines. Cleaned up BasePrompt: removed try-catch from 'set value' as we assign the value even if there is an error thrown, changed the fulfill() so that it throws an error if the value is invalid which means the flow runner doesn't proceed/run with an invalid value anymore. Updated tests and readme file.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (selections.length === 0) { | ||
| throw new ValidationException(INVALID_AT_LEAST_ONE_SELECTION_REQUIRED) | ||
| if (choices.length !== 0 && selections.length === 0) { | ||
| throw new PromptValidationException(INVALID_AT_LEAST_ONE_SELECTION_REQUIRED) | ||
| } | ||
|
|
||
| const invalidChoices = difference(selections, map(choices, 'key')) |
There was a problem hiding this comment.
Reporter: ESLint
Rule: eslint.rules.lodash/prop-shorthand
Severity: WARN
File: src/domain/prompt/SelectManyPrompt.ts L47
Do not use property shorthand syntax (lodash/prop-shorthand)
|
Some changes (e.g. spaces removed) are not mine - they are from 'prettier', I believe |
| @@ -91,25 +72,29 @@ export abstract class BasePrompt<T extends IPromptConfig<T['value']>> implements | |||
| } | |||
|
|
|||
| async fulfill(val: T['value'] | undefined): Promise<IRichCursorInputRequired | undefined> { | |||
| // allow prompt.fulfill() for continuation | |||
There was a problem hiding this comment.
@bulatgab We should change this comment to say; "We need to exempt setting this.value when prompt.fulfill() is called without any arguments, because it would reset our state to "uninitialized" due to val being undefined."
There was a problem hiding this comment.
Thanks, Brett. It's clearer that way indeed. I will change
…he changes: removed validateOrThrow (now there is validate() that returns void or throws, and isValid() that returns boolean), added InvalidChoiceException back (but now it extends PromptValidationException). Updated NumericPrompt validate() using lodash isFinite().
| @@ -59,20 +59,20 @@ describe('SelectManyPrompt', () => { | |||
|
|
|||
| it('should raise when no selections are provided', async () => { | |||
There was a problem hiding this comment.
Reporter: ESLint
Rule: eslint.rules.jest/expect-expect
Severity: WARN
File: src/tests/prompt/SelectManyPrompt.spec.ts L60
Test has no assertions (jest/expect-expect)
| @@ -60,5 +60,5 @@ export interface IBasePromptConfig { | |||
| } | |||
|
|
|||
| export interface PromptConstructor<T> { | |||
| new(config: T, interactionId: string, runner: IFlowRunner): BasePrompt<any> | |||
| new (config: T, interactionId: string, runner: IFlowRunner): BasePrompt<any> | |||
There was a problem hiding this comment.
Reporter: ESLint
Rule: eslint.rules.@typescript-eslint/no-explicit-any
Severity: WARN
File: src/domain/prompt/IPrompt.ts L63
Unexpected any. Specify a different type. (@typescript-eslint/no-explicit-any)
… the project, built now. Plus a minor change in BasePrompt.spec.ts (refactored the test back to the original form) and BasePrompt.ts (elaborated the code comment using Brett's suggestion in PR's conversation).
|
Found 8 violations: Reporter: ESLint Missing return type on function. (@typescript-eslint/explicit-function-return-type)
Reporter: ESLint Test has no assertions (jest/expect-expect)
Reporter: ESLint Test has no assertions (jest/expect-expect)
Reporter: ESLint Test has no assertions (jest/expect-expect)
Reporter: ESLint Unexpected any. Specify a different type. (@typescript-eslint/no-explicit-any)
Reporter: ESLint Unexpected any. Specify a different type. (@typescript-eslint/no-explicit-any)
Reporter: ESLint Unexpected any. Specify a different type. (@typescript-eslint/no-explicit-any)
Reporter: ESLint Do not use property shorthand syntax (lodash/prop-shorthand)
|
