-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
Resolved issue #3359 #6969
Resolved issue #3359 #6969
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logically seems fine. I appreciate that the default behavior is preserved.
My one request/niggle is that, instead of sever
the new method/private be named close
(closeConnectionsOnUpdate
, _closeConnectionsOnUpdate
) just for sanity's sake. sever
and server
look very much alike, and we use the term close
for all methods which close TCP connections, not sever
Also, please add a little note about this new method in doc/ota_updates
with @igrr's warning and your use case. OTW It's rather hard for someone to figure out what/why to use this...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @earlephilhower.
@timstableford please rename sever => close, and we can merge. All else looks ok to me.
Made severing connections optional as per the patch in the issue. Also fixed a minor spacing issue.
Also my editor automatically removed some odd whitespace at the end of a few lines.
17c74ff
to
4d2b71c
Compare
Changes now done. Please let me know if there's any issues with the detail added in the readme. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Made severing connections optional as per the patch in the issue.
Also fixed a minor spacing issue.
Fixes #3359