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

introduces onClose listener for RSocketClient and connect method #1063

Merged
merged 2 commits into from Sep 14, 2022

Conversation

OlegDokuka
Copy link
Member

closes #1048

Signed-off-by: Oleh Dokuka odokuka@vmware.com
Signed-off-by: Oleh Dokuka oleh.dokuka@icloud.com

@OlegDokuka OlegDokuka added this to the 1.1.3 milestone Aug 30, 2022
@OlegDokuka OlegDokuka changed the title introduces listener for RSocketClient and start method introduces listener for RSocketClient and connect method Aug 31, 2022
@OlegDokuka OlegDokuka changed the title introduces listener for RSocketClient and connect method introduces onClose listener for RSocketClient and connect method Aug 31, 2022
Copy link
Contributor

@rstoyanchev rstoyanchev left a comment

Choose a reason for hiding this comment

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

Looks good. Only a couple of small comments for the connect method, which could also be split out into a separate commit as it is orthogonal to the onClose I think.


default boolean connect() {
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should raise NotImplementedException or not have a default implementation at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

It also needs Javadoc to state what it does, when this is useful vs just making requests, when it returns true vs false, and what happens for any scenarios of interest (e.g. if already connected, if already terminated, etc).

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
Signed-off-by: Oleh Dokuka <oleh.dokuka@icloud.com>
@OlegDokuka OlegDokuka force-pushed the enhancement/1.1.x-client-start-onclose branch from 14fbd84 to 40e02a5 Compare September 9, 2022 10:08
@OlegDokuka
Copy link
Member Author

@rstoyanchev added 2 separate commits as it was suggested

@OlegDokuka OlegDokuka force-pushed the enhancement/1.1.x-client-start-onclose branch 2 times, most recently from b7b3c4a to d1875da Compare September 9, 2022 17:17
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
Signed-off-by: Oleh Dokuka <oleh.dokuka@icloud.com>
Co-authored-by: Rossen Stoyanchev <rstoyanchev@users.noreply.github.com>
Signed-off-by: Oleh Dokuka <oleh.dokuka@icloud.com>
@OlegDokuka OlegDokuka force-pushed the enhancement/1.1.x-client-start-onclose branch from d1875da to 7502fde Compare September 9, 2022 17:54
@OlegDokuka OlegDokuka merged commit 01f5458 into 1.1.x Sep 14, 2022
@OlegDokuka OlegDokuka deleted the enhancement/1.1.x-client-start-onclose branch September 14, 2022 08:41
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.

None yet

2 participants