Skip to content

Add validate cli tool#9

Open
0marSalah wants to merge 7 commits intomainfrom
add-validate-cli-tool
Open

Add validate cli tool#9
0marSalah wants to merge 7 commits intomainfrom
add-validate-cli-tool

Conversation

@0marSalah
Copy link
Copy Markdown
Collaborator

No description provided.

@0marSalah
Copy link
Copy Markdown
Collaborator Author

USAGE

  • wallet-export validate <path-to-export.tsr>

image

src/index.ts Outdated
export async function importActorProfile(
tarStream: Readable
): Promise<Record<string, any>> {
console.log('🚀 Starting to process tar stream...')
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Will you make it so console can be injected as an argument?

And this line like console?.log(....)

that way, by default, this function won't console.log, which is nice outside of the CLI context.

The CLI can inject in { console: globalThis.console } when it calls these functions so that, only in the cli context, this function produces logs. But then there is also a way to opt-out of verbose logging.

src/index.ts Outdated
}
} catch (error: any) {
reject(new Error(`Error processing file ${fileName}: ${error}`))
console.error(`Error processing file ${fileName}:`, error.message)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If there is an error, imho this function should throw so the caller can catch it.
(the caller might catch it and log, but it also might not want to log).

It could also be useful to allow an onError callback to optionally be passed that gets passed with each error. If that is provided, you can call it, it can decide whether to throw or not.

But I definitely think it is always a little risky to catch an error and only log it. Would be better to throw or pass to a callback.

src/index.ts Outdated
stream.on('error', (error: any) => {
reject(new Error(`Stream error on file ${fileName}: ${error}`))
console.error(`Stream error on file ${fileName}:`, error.message)
next() // Continue even on stream error
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it probably makes sense to pass the error to next?

src/index.ts Outdated
})

tarStream.on('error', (error) => {
console.error('Error in tar stream:', error.message)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A lot of these logs could probably be moved into cli.ts and listen for error events on the stream in there.

imho we should only console.log in the cli invocation of this function, not every time this function is called

@0marSalah 0marSalah force-pushed the add-validate-cli-tool branch from e5bb242 to 89f761e Compare February 18, 2025 11:01
@0marSalah
Copy link
Copy Markdown
Collaborator Author

Resolved the rebase conflict, now its done.

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.

2 participants