Skip to content

Conversation

luisteod
Copy link

What changes were proposed in this pull request?

This PR fix typos in ExecuteGrpcResponseSender.scala

Why are the changes needed?

For readability improvement

Does this PR introduce any user-facing change?

No

How was this patch tested?

Run manual tests

Was this patch authored or co-authored using generative AI tooling?

No

@@ -75,7 +76,7 @@ private[connect] class ExecuteGrpcResponseSender[T <: Message](
}

// For testing
private[connect] def setDeadline(deadlineNs: Long) = {
private[connect] def setDeadline(deadlineNs: Long): Unit = {
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to give you just one tiny advice to you, @luisteod . Please don't mix irrelevant themes into a single PR. This is not a typo fix.

Copy link
Author

Choose a reason for hiding this comment

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

Got it! May I just leave the comments typo fix?

Copy link
Member

Choose a reason for hiding this comment

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

Simply revert this line change by removing : Unit back?

Copy link
Author

Choose a reason for hiding this comment

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

Updated

@@ -300,10 +301,10 @@ private[connect] class ExecuteGrpcResponseSender[T <: Message](
if (sent) {
sentResponsesSize += response.get.serializedByteSize
nextIndex += 1
assert(finished == false)
assert(!finished)
Copy link
Member

Choose a reason for hiding this comment

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

ditto. This is not a type fix.

Copy link
Author

Choose a reason for hiding this comment

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

Updated

@luisteod luisteod requested a review from dongjoon-hyun August 28, 2025 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants