Conversation
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Signed-off-by: John-David Dalton <jdalton@users.noreply.github.com>
Signed-off-by: John-David Dalton <jdalton@users.noreply.github.com>
| `Socket API server error (${statusCode}): ${statusMessage}`, | ||
| ) | ||
| } else { | ||
| const { spinner } = constants |
There was a problem hiding this comment.
The codebase appears to be using a style where semicolons are optional. Since this is consistent with the surrounding code, adding an explicit semicolon here isn't necessary. If there's a specific linting rule or style guide requiring semicolons, consider applying it consistently across the entire codebase rather than to individual lines.
Spotted by Diamond (based on custom rules)
Is this helpful? React 👍 or 👎 to let us know.
| ) | ||
| } else { | ||
| const { spinner } = constants | ||
| spinner.stop() |
There was a problem hiding this comment.
The codebase appears to be using a style where semicolons are optional. Since this is consistent with the surrounding code (which also doesn't use semicolons), adding one here would actually be inconsistent with the project's style. Unless there's a specific linting rule for this project requiring semicolons, this code follows what appears to be the established convention.
Spotted by Diamond (based on custom rules)
Is this helpful? React 👍 or 👎 to let us know.
| logger.fail( | ||
| `Received a ${batchResult.status} response from Socket API which we consider a permanent failure:`, | ||
| batchResult.error, | ||
| batchResult.cause ? `( ${batchResult.cause} )` : '', | ||
| ) |
There was a problem hiding this comment.
Consider adding a semicolon at the end of the logger.fail statement for consistency. While JavaScript has automatic semicolon insertion, explicitly terminating statements helps maintain code clarity, especially in multi-line function calls.
logger.fail(
`Received a ${batchResult.status} response from Socket API which we consider a permanent failure:`,
batchResult.error,
batchResult.cause ? `( ${batchResult.cause} )` : '',
);| logger.fail( | |
| `Received a ${batchResult.status} response from Socket API which we consider a permanent failure:`, | |
| batchResult.error, | |
| batchResult.cause ? `( ${batchResult.cause} )` : '', | |
| ) | |
| logger.fail( | |
| `Received a ${batchResult.status} response from Socket API which we consider a permanent failure:`, | |
| batchResult.error, | |
| batchResult.cause ? `( ${batchResult.cause} )` : '', | |
| ) |
Spotted by Diamond (based on custom rules)
Is this helpful? React 👍 or 👎 to let us know.
| batchResult.error, | ||
| batchResult.cause ? `( ${batchResult.cause} )` : '', | ||
| ) | ||
| break |
There was a problem hiding this comment.
The break statement is missing a semicolon. While JavaScript's automatic semicolon insertion will handle this case correctly, adding an explicit semicolon would maintain consistency with the codebase's style conventions.
| break | |
| break; |
Spotted by Diamond (based on custom rules)
Is this helpful? React 👍 or 👎 to let us know.
2 participants
This requires SocketDev/socket-sdk-js#325
That fix propagates an error response, which is returned as the generator's final (done:true) value. But this value was always ignored.
This PR will pick up the response and even when nothrow is set, when the response is 401 or 404 it will consider it a permanent failure.
(I think it should do this regardless but I'll leave that up to @jdalton to refine)