Skip to content
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

natsConnection_Buffered: fix returned value in case of error #650

Merged
merged 1 commit into from
May 2, 2023

Conversation

kolomenkin
Copy link
Contributor

This function returns number of bytes in the buffer.
It is incorrect to return value from natsStatus enum.

We could keep setting internal error code.
But in this case we should set error code for other cases too where we return -1

@kozlovic
Copy link
Member

kozlovic commented May 2, 2023

@kolomenkin Thank you fro your contribution. It looks like you based your fix from the dev branch but posted against main branch. If you could update/re-create your PR from the main branch instead - since this is a fix and should go in the main branch, that would be great!

@kolomenkin kolomenkin changed the base branch from main to dev May 2, 2023 21:56
@kolomenkin
Copy link
Contributor Author

@kozlovic , sorry, my mistake. I knew I should create PR against dev, but forgot to ensure this.
I updated PR's target branch few moments ago.

@kolomenkin kolomenkin changed the base branch from dev to main May 2, 2023 22:00
@kozlovic
Copy link
Member

kozlovic commented May 2, 2023

@kolomenkin But actually no, the way we are doing it (maybe wrong, but that's our procedure), is that a fix should go against main branch, so that it will be available for any "patch" release if one is needed. However, when adding a new feature, or breaking changes, then it should be submitted against dev so that it goes only in the next major/minor release.

I would qualify your change as a fix, so it should be done against main. Thank you for your understanding!

@kozlovic
Copy link
Member

kozlovic commented May 2, 2023

At this point, from your fork, make sure that you are up-to-date with the upstream main branch and create a PR with that one line change. There should not be 7 commits like there is in this PR.

This function returns number of bytes in the buffer.
It is incorrect to return value from natsStatus enum.
@kolomenkin
Copy link
Contributor Author

@kozlovic , sorry, my first PR fix was incorrect.
Now I rebased my commit and branch on top of latest master branch.
So now there is only 1 commit in PR.

@kozlovic kozlovic self-requested a review May 2, 2023 22:26
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for your contribution!

@kozlovic kozlovic merged commit 7f6910e into nats-io:main May 2, 2023
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.

None yet

2 participants