Skip to content

Mark scan() and scanAll() parameters as optional #564

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

Merged
merged 2 commits into from
Mar 19, 2025

Conversation

sjorsrijsdam
Copy link
Contributor

The documentation for the scan() and scanAll() methods (https://arc.codes/docs/en/reference/runtime-helpers/node.js#instance-methods) say that the parameters are optional. However, this isn't reflected in the type definitions. This PR fixes that.

Copy link
Member

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

Great improvement, thank you for the PR! I do see a problem with your approach, though, so I will fork from you branch and open a new PR with the adjustment in order to maintain your changes ooo I can just push to your fork branch - nice!

scan(params: ScanParams): Promise<ScanOutput<Item>>;
scan(params: ScanParams, callback: Callback<ScanOutput<Item>>): void;
scan(params?: ScanParams): Promise<ScanOutput<Item>>;
scan(params?: ScanParams, callback: Callback<ScanOutput<Item>>): void;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think a required parameter can follow an optional parameter. I would probably split this out into two further function signatures.

I'll fork from your branch and send a new PR with an adjustment, to maintain your credit.

Copy link
Member

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

Good job team!

@filmaj filmaj merged commit cb65463 into architect:main Mar 19, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants