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

Implement deleteStream command on client connection #56

Merged
merged 2 commits into from Jul 1, 2023

Conversation

jbreich
Copy link
Contributor

@jbreich jbreich commented Jun 16, 2023

Hello, I implemented the possibility to send a deleteStream command on a client connection. The aim is to properly end a stream. I am not sure it is the best way to do it so I am open to your feedbacks.

@codecov
Copy link

codecov bot commented Jun 17, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.86 ⚠️

Comparison is base (db41187) 62.29% compared to head (1d3bce5) 61.44%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #56      +/-   ##
==========================================
- Coverage   62.29%   61.44%   -0.86%     
==========================================
  Files          36       36              
  Lines        1870     1896      +26     
==========================================
  Hits         1165     1165              
- Misses        568      594      +26     
  Partials      137      137              
Flag Coverage Δ
unittests 61.44% <0.00%> (-0.86%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
client_conn.go 53.33% <0.00%> (-16.24%) ⬇️
message/net_stream.go 50.76% <0.00%> (-2.46%) ⬇️
stream.go 60.00% <0.00%> (-3.36%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Owner

@yutopp yutopp left a comment

Choose a reason for hiding this comment

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

Thank you for the pull request! I will merge it.
In order to increase coverage, I plan to add tests, including for existing code, and would like to create a release tag either today or tomorrow. (If this is urgent, please let me know in the comments.)

@jbreich
Copy link
Contributor Author

jbreich commented Jun 17, 2023

Thank you for your quick reponse. It is not urgent no problem

@jbreich
Copy link
Contributor Author

jbreich commented Jun 21, 2023

I added a commit about a leak on the client connection, sorry. I can put that in another branch if you want ?

@yutopp
Copy link
Owner

yutopp commented Jul 1, 2023

I apologize for the delay in my action. No problem at all! Thank you for your additional corrections.

@yutopp yutopp merged commit 9a7a975 into yutopp:master Jul 1, 2023
2 of 4 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.

None yet

2 participants