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

add error handling in go2cStr #55

Merged
merged 2 commits into from
Mar 9, 2023
Merged

add error handling in go2cStr #55

merged 2 commits into from
Mar 9, 2023

Conversation

PiotrLewandowski323
Copy link
Contributor

fixes #50

@coveralls
Copy link
Collaborator

coveralls commented Mar 6, 2023

Coverage Status

Coverage: 74.438% (+0.7%) from 73.784% when pulling 4e3dd9a on PiotrLewandowski323:issue-50 into bce2fcc on roc-streaming:main.

@ortex
Copy link
Member

ortex commented Mar 7, 2023

LGTM. But, it would be great to add tests for endpoint, to cover new branches.

For sender and receiver there are separate issues

@gavv
Copy link
Member

gavv commented Mar 7, 2023

Minor comment: when we're forwarding error from go2cStr, like this:

	cIP, err := go2cStr(ip)
	if err != nil {
		return err
	}

it'd be nice to wrap the returned error and explain what string was invalid (otherwise it would be hard to guess for user). Like:

return fmt.Errorf("invalid ip: %w", err)

@PiotrLewandowski323
Copy link
Contributor Author

The linux build in CI uses go 1.12 which doesn't support "%w" to wrap errors, so I had to replace it with "%v".

@gavv
Copy link
Member

gavv commented Mar 8, 2023

I think we can bump minimum go to 1.13 (in go mod and .github/workflows). The choice of 1.12 with rather arbitrary (it just was known to work on 1.12). On the other hand, I'd say %w is a very very useful feature for error handling.

Copy link
Member

@gavv gavv left a comment

Choose a reason for hiding this comment

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

Thanks for update! A couple more (I think final) comments.

roc/endpoint.go Outdated Show resolved Hide resolved
roc/convert.go Outdated Show resolved Hide resolved
roc/endpoint_test.go Outdated Show resolved Hide resolved
Signed-off-by: Piotr Lewandowski <lewandowski323@gmail.com>
Signed-off-by: Piotr Lewandowski <lewandowski323@gmail.com>
@PiotrLewandowski323
Copy link
Contributor Author

I rebased onto main because endpoint tests were refactored and updated the changes according to your comments.

@gavv gavv merged commit 7d9df77 into roc-streaming:main Mar 9, 2023
@gavv
Copy link
Member

gavv commented Mar 9, 2023

Thanks!!

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.

Add error handling to go2cStr
4 participants