8000 refactor: goods refactoring by antongolub · Pull Request #1195 · google/zx · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

refactor: goods refactoring #1195

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 10 commits into from
Apr 13, 2025
Merged

refactor: goods refactoring #1195

merged 10 commits into from
Apr 13, 2025

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 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)

@@ -224,10 +224,12 @@ export function getCallerLocationFromString(stackString = 'unknown'): string {
const lines = stackString
.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
Preview
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
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.

1 participant
0