-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: Skips failed imported items, supports detailed log in console / FS indicating what failed and why, extends result models for import
functionality
#21
Conversation
…d processing requests
…file within current folder
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
lib/core/models/core.models.ts
Outdated
@@ -145,3 +145,9 @@ export interface OriginalManagementError { | |||
}; | |||
}; | |||
} | |||
|
|||
export type ItemProcessingResult<InputItem, OutputItem> = { |
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 think you could use export type ItemProcessingResult<InputItem, OutputItem> = Readonly<{...}>
, then you don't have to write readonly
in front of every property.
This is more of a personal taste, so I keep it up to you :)
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.
Also I would make this type as discriminated union
export type ItemProcessingResult<InputItem, OutputItem> = Readonly<{
success: true;
inputItem: InputItem;
outputItem: OutputItem;
}> | Readonly<{
success: false;
inputItem: InputItem;
error: unknown;
}>
It would help differentiate when there is outputItem and where there is error in the code :)
but this would need some refactor in other places too
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.
Nice catch :) I've used discriminated union for the ItemProcessingResult
. I only needed to 3 states so I've changed the boolean
to a type literal.
import { assetsImporter } from './importers/assets-importer.js'; | ||
import { contentItemsImporter } from './importers/content-items-importer.js'; | ||
import { languageVariantImporter } from './importers/language-variant-importer.js'; | ||
|
||
const reportFilename: string = `import-report.json`; | ||
|
||
type ReportResult = { |
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 got this to chatgpt to rewrite it using Readonly<>
type ReportResult = Readonly<{
errorsCount: number;
assets: Readonly<{
count: number;
successful: ReadonlyArray<Readonly<{ codename: string }>>;
failed: ReadonlyArray<Readonly<{ codename: string; error: string }>>;
}>;
languageVariants: Readonly<{
count: number;
successful: ReadonlyArray<Readonly<{
codename: string;
language: Readonly<{ codename: string }>;
type: Readonly<{ codename: string }>;
}>>;
failed: ReadonlyArray<Readonly<{
codename: string;
language: Readonly<{ codename: string }>;
type: Readonly<{ codename: string }>;
error: string;
}>>;
}>;
contentItems: Readonly<{
count: number;
successful: ReadonlyArray<Readonly<{ codename: string }>>;
failed: ReadonlyArray<Readonly<{
codename: string;
type: Readonly<{ codename: string }>;
error: string;
}>>;
}>;
}>;
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.
It may be ideal to look into a DeepReadonly
utility type.
Something like the following: https://stackoverflow.com/questions/41879327/deepreadonly-object-typescript
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.
Even though it is a nice type function I don't think that's necessary :)
The type is not that nested it would need this :D
Motivation
Which issue does this fix? Fixes #
issue number
If no issue exists, what is the fix or new feature? Were there any reasons to fix/implement things that are not obvious?
Checklist
How to test
If manual testing is required, what are the steps?