-
Notifications
You must be signed in to change notification settings - Fork 2.9k
fix(types): make result a strong type
#39783
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
base: main
Are you sure you want to change the base?
Conversation
viceice
left a comment
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.
some small suggestions
| REPOSITORY_DISABLED, | ||
| REPOSITORY_DISABLED_BY_CONFIG, | ||
| REPOSITORY_NO_CONFIG, | ||
| ]; |
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.
you can use as const here to make typescript happy
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.
I can do i.e.
const disabledMessages = [
REPOSITORY_CLOSED_ONBOARDING,
REPOSITORY_DISABLED,
REPOSITORY_DISABLED_BY_CONFIG,
REPOSITORY_NO_CONFIG,
] as const;
if (disabledMessages.some((m) => err.message === m)) {
logger.info('Repository is disabled - skipping');
return err.message;
}But I see:
lib/workers/repository/error.ts:67:5 - error TS2322: Type 'string' is not assignable to type 'RepositoryResult'.
67 return err.message;
~~~~~~
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.
that's unfortunate 🫣
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.
ah. use includes instead of some
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.
ah. use includes instead of some
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.
Using includes:
const disabledMessages = [
REPOSITORY_CLOSED_ONBOARDING,
REPOSITORY_DISABLED,
REPOSITORY_DISABLED_BY_CONFIG,
REPOSITORY_NO_CONFIG,
] as const;
if (disabledMessages.includes(err.message)) {
logger.info('Repository is disabled - skipping');
return err.message;
}results in:
lib/workers/repository/error.ts:65:33 - error TS2345: Argument of type 'string' is not assignable to parameter of type '"disabled" | "disabled-closed-onboarding" | "disabled-by-config" | "disabled-no-config"'.
65 if (disabledMessages.includes(err.message)) {
~~~~~~~~~~~
lib/workers/repository/error.ts:67:5 - error TS2322: Type 'string' is not assignable to type 'RepositoryResult'.
67 return err.message;
~~~~~~
viceice
left a comment
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.
see comments
c7ea5b0 to
ce06a68
Compare
As a way to make it more predictable what the possible states the `result` in a given `Repository finished` log message can be, we can wrap these into a type. This requires a slight refactor where these are used to make sure that Typescript is happy with the value being returned. We can capture the common groups of errors (which are then present as a `RepositoryResult`) and expose that to be used.
ce06a68 to
be0abad
Compare
Changes
As a way to make it more predictable what the possible states the
resultin a givenRepository finishedlog message can be, we canwrap these into a type.
This requires a slight refactor where these are used to make sure that
Typescript is happy with the value being returned.
Context
Please select one of the below:
AI assistance disclosure
Did you use AI tools to create any part of this pull request?
Please select one option and, if yes, briefly describe how AI was used (e.g., code, tests, docs) and which tool(s) you used.
Documentation (please check one with an [x])
How I've tested my work (please select one)
I have verified these changes via:
The public repository: