-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add IntRange type for ColorIndex #4618
Conversation
src/common/InputHandler.ts
Outdated
function isValidColorIndex(value: number): value is ColorIndex { | ||
return 0 <= value && value < 259; | ||
} |
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.
@jerch should this be < 256 instead? If so what would be a good name for this color index sub-range? IsValidInputColorIndex
or something?
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.
Yes, for colors the index is only 0..255, so its better to test for that range in during input. The fg/bg/cursor entries on top are an amalgamation, so the events and methods dont need re-declaration and/or branching.
src/common/Types.d.ts
Outdated
: Enumerate<N, [...Acc, Acc['length']]>; | ||
type IntRange<F extends number, T extends number> = Exclude<Enumerate<T>, Enumerate<F>>; | ||
|
||
type ColorIndex = IntRange<0, 259>; // number from 0 to 258 |
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.
Would it be clearer to limit ColorIndex
here to 0..255, and to merge into a super type with the fg/bg/cursor values in a second step? Also the test function above can then refer to that narrower ColorIndex
type.
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.
LGTM, beside the left-over doubled change check below.
Fixes #4614