Skip to content

refactor: goods refactoring#1195

Merged
antongolub merged 10 commits intogoogle:mainfrom
antongolub:goods-refactor
Apr 13, 2025
Merged

refactor: goods refactoring#1195
antongolub merged 10 commits intogoogle:mainfrom
antongolub:goods-refactor

Conversation

@antongolub
Copy link
Collaborator

This PR:

  • Fixes expBackoff implementation
  • Removes redundant goods chunk
  • Makes configurable question() I/O
  • Sets $.log.output as default spinner output
  • Slightly reduces the pkg size
  • Tests pass
  • Appropriate changes to README are included in PR

@antongolub antongolub requested a review from Copilot April 13, 2025 19:48
Copy link

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.

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

Files not reviewed (3)
  • .nycrc: Language not supported
  • .size-limit.json: Language not supported
  • package.json: Language not supported
Comments suppressed due to low confidence (1)

src/goods.ts:227

  • The refactored expBackoff implementation now computes the delay as 'randMs * 2 ** n' without any random jitter, whereas the previous version added a random component. Please confirm that removing the randomness is intentional, as this changes the backoff behavior.
const randMs = parseDuration(delay)

.split(/^\s*(at\s)?/m)
.filter((s) => s?.includes(':'))
const i = lines.findIndex((l) => l.includes('Proxy.set'))
const offset = i < 0 ? i : i + 2
Copy link

Copilot AI Apr 13, 2025

Choose a reason for hiding this comment

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

Using a negative index for 'offset' when 'Proxy.set' is not found may lead to undefined behavior in the caller location lookup. Consider setting a default positive offset instead of passing a negative one.

Suggested change
const offset = i < 0 ? i : i + 2
const offset = i < 0 ? 0 : i + 2

Copilot uses AI. Check for mistakes.
@antongolub antongolub merged commit 92227da into google:main Apr 13, 2025
28 checks passed
antongolub added a commit to antongolub/zx that referenced this pull request Jun 25, 2025
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