Skip to content

Conversation

@bulatgab
Copy link
Collaborator

@bulatgab bulatgab commented Jan 20, 2021

  • 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.
@bulatgab bulatgab requested review from bzabos and seifertk January 20, 2021 22:17
throw new PromptValidationException(INVALID_AT_LEAST_ONE_SELECTION_REQUIRED)
}

const invalidChoices = difference(selections, map(choices, 'key'))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@bulatgab
Copy link
Collaborator Author

Some changes (e.g. spaces removed) are not mine - they are from 'prettier', I believe

}

async fulfill(val: T['value'] | undefined): Promise<IRichCursorInputRequired | undefined> {
// allow prompt.fulfill() for continuation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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."

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 () => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


export interface PromptConstructor<T> {
new(config: T, interactionId: string, runner: IFlowRunner): BasePrompt<any>
new (config: T, interactionId: string, runner: IFlowRunner): BasePrompt<any>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).
@github-actions
Copy link

Found 8 violations:

Reporter: ESLint
Rule: eslint.rules.@typescript-eslint/explicit-function-return-type
Severity: WARN
File: src/tests/prompt/SelectManyPrompt.spec.ts L80

Missing return type on function. (@typescript-eslint/explicit-function-return-type)

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)

Reporter: ESLint
Rule: eslint.rules.jest/expect-expect
Severity: WARN
File: src/tests/prompt/SelectManyPrompt.spec.ts L44

Test has no assertions (jest/expect-expect)

Reporter: ESLint
Rule: eslint.rules.jest/expect-expect
Severity: WARN
File: src/tests/prompt/SelectManyPrompt.spec.ts L33

Test has no assertions (jest/expect-expect)

Reporter: ESLint
Rule: eslint.rules.@typescript-eslint/no-explicit-any
Severity: WARN
File: src/tests/prompt/SelectManyPrompt.spec.ts L25

Unexpected any. Specify a different type. (@typescript-eslint/no-explicit-any)

Reporter: ESLint
Rule: eslint.rules.@typescript-eslint/no-explicit-any
Severity: WARN
File: src/domain/prompt/BasePrompt.ts L32

Unexpected any. Specify a different type. (@typescript-eslint/no-explicit-any)

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)

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants