Skip to content

Conversation

@NubeDev
Copy link

@NubeDev NubeDev commented Nov 23, 2025

see issue #61

Copilot AI review requested due to automatic review settings November 23, 2025 01:52
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes an issue where the writeFile method couldn't properly set the file access type (stream vs record-oriented). The fix introduces a new WriteFileOptions interface that extends ServiceOptions with an isStream boolean parameter, allowing callers to explicitly specify whether they want stream-based or record-based file access.

Key changes:

  • Added WriteFileOptions interface with optional isStream parameter
  • Updated writeFile method to use WriteFileOptions and properly handle the isStream option
  • Default behavior now explicitly uses stream mode when not specified

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/lib/types.ts Introduces WriteFileOptions interface extending ServiceOptions with isStream parameter
src/lib/client.ts Updates writeFile method to accept WriteFileOptions, extracts isStream value with stream mode as default, and passes it to the encoder

const settings = {
maxSegments:
(options as ServiceOptions).maxSegments ||
(options as WriteFileOptions).maxSegments ||
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

The type assertion (options as WriteFileOptions) is redundant since options is already typed as WriteFileOptions in the function signature. Remove the type assertions on lines 1290, 1293, and 1296.

Copilot uses AI. Check for mistakes.
@robertsLando robertsLando changed the title fix cant set file type as isStream fix: set file type as isStream Nov 24, 2025
Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

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

@NubeDev Thanks for your PR! @jacoscaz Do you agree with the new default?

@jacoscaz
Copy link
Contributor

Yep, sounds good to me @robertsLando . Thank you @NubeDev for the PR!

Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

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

@NubeDev I see there are some lint issues, could you run npm run lint:fix and push new changes please?

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.

3 participants