refactor(prompt): migrate prompt to typescript #2371
Conversation
| @@ -46,7 +46,7 @@ export default { | |||
| }, | |||
| body: { | |||
| description: '<body> holds additional information about the change', | |||
| multline: true, | |||
| multiline: true, | |||
There was a problem hiding this comment.
not sure if i should fix this, as previous field was never used
There was a problem hiding this comment.
I think this is good to leave. The body can actually be multiline, but we don't have that many users running into this. If your body is just one line of text, you will never use it anyways 😉
| return (input) => input.toLowerCase() === input; | ||
| return (input: string) => input.toLowerCase(); |
There was a problem hiding this comment.
in case of lowecase this code was producing invalid result
There was a problem hiding this comment.
i found out why this code did not crash for anyone, few lines above we have:
const [config] = rule;
if (!Array.isArray(config)) {
return noop;
}and this condition never passes as we should use
const [, config] = rule;
| throw new TypeError(`Unknown target case "${rule[2]}"`); | ||
| throw new TypeError(`Unknown target case "${target}"`); |
There was a problem hiding this comment.
rule[2] is never defined, it has to be config[2] or target to print correct value
There was a problem hiding this comment.
Good catch again, I think you are right with the target here. Looks like target is also config[2] 😄
| export interface PrompterCommand { | ||
| description(value: string): this; | ||
| action( | ||
| action: (args: { | ||
| [key: string]: any; | ||
| options: { | ||
| [key: string]: any; | ||
| }; | ||
| }) => Promise<void> | void | ||
| ): this; | ||
| } | ||
|
|
||
| export interface Prompter { | ||
| delimiter(value: string): this; | ||
| show(): this; | ||
| exec(command: string): Promise<any>; | ||
| log(text?: string): void; | ||
| catch(command: string, description?: string): PrompterCommand; | ||
| command(command: string, description?: string): PrompterCommand; | ||
| removeAllListeners(input?: string): void; | ||
| addListener(input: string, cb: (event: any) => void): void; | ||
| ui: { | ||
| log(text?: string): void; | ||
| input(text?: string): string; | ||
| redraw: { | ||
| (text: string, ...texts: string[]): void; | ||
| done(): void; | ||
| }; | ||
| }; | ||
| } |
There was a problem hiding this comment.
it will be nice if we could use vorpal types instead but, @types/vorpal is not correct
There was a problem hiding this comment.
That's unfortunate, thanks for adding the types anyways!
| @@ -46,7 +46,7 @@ export default { | |||
| }, | |||
| body: { | |||
| description: '<body> holds additional information about the change', | |||
| multline: true, | |||
| multiline: true, | |||
There was a problem hiding this comment.
I think this is good to leave. The body can actually be multiline, but we don't have that many users running into this. If your body is just one line of text, you will never use it anyways 😉
| export interface PrompterCommand { | ||
| description(value: string): this; | ||
| action( | ||
| action: (args: { | ||
| [key: string]: any; | ||
| options: { | ||
| [key: string]: any; | ||
| }; | ||
| }) => Promise<void> | void | ||
| ): this; | ||
| } | ||
|
|
||
| export interface Prompter { | ||
| delimiter(value: string): this; | ||
| show(): this; | ||
| exec(command: string): Promise<any>; | ||
| log(text?: string): void; | ||
| catch(command: string, description?: string): PrompterCommand; | ||
| command(command: string, description?: string): PrompterCommand; | ||
| removeAllListeners(input?: string): void; | ||
| addListener(input: string, cb: (event: any) => void): void; | ||
| ui: { | ||
| log(text?: string): void; | ||
| input(text?: string): string; | ||
| redraw: { | ||
| (text: string, ...texts: string[]): void; | ||
| done(): void; | ||
| }; | ||
| }; | ||
| } |
There was a problem hiding this comment.
That's unfortunate, thanks for adding the types anyways!
| return (input) => input.toLowerCase() === input; | ||
| return (input: string) => input.toLowerCase(); |
| throw new TypeError(`Unknown target case "${rule[2]}"`); | ||
| throw new TypeError(`Unknown target case "${target}"`); |
There was a problem hiding this comment.
Good catch again, I think you are right with the target here. Looks like target is also config[2] 😄
| const inputMaxLength = hasMaxLength ? maxLenghtRule[1][1] : Infinity; | ||
| const inputMaxLength = | ||
| maxLengthRule && | ||
| maxLengthRule[1] && | ||
| maxLengthRule[1][0] > 0 && | ||
| typeof maxLengthRule[1][1] === 'number' | ||
| ? maxLengthRule[1][1] | ||
| : Infinity; |
There was a problem hiding this comment.
there is one more issue here, this code should be same as in input, right now is going to always return Infinity
[name, [severity, applicable, length]]instead of reading maxLengthRule[1][1] e should read maxLengthRule[1][2] and check if rule is applicable
or write helper like this one and use it in input and here:
export function getMaxLength (rule?: RuleEntry): number {
if (rule && ruleIsActive(rule) && ruleIsApplicable(rule) && typeof rule[1][2] === 'number') {
return rule[1][2]
}
return Infinity
}
|
I created some tests for this package and fixed some of known issues #608 |
armano2
changed the title
Description
Part of #659, last package that is compiled with babel, main issue is with types of vorpal, this package has not been updated for few years and definition for it in DefinedTypes is really bad
Changes in this PR has been limited to only necessary type addition as refactoring of this code without proper tests is really dangerous
Motivation and Context
Ported prompt to typescript as part of #659
fixes #608
How Has This Been Tested?
Manual tests and unit tests from project
tests in this package are very limited
Types of changes
Checklist: