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

Feature: Support adding service endpoint after construction #1274

Conversation

rexf
Copy link
Contributor

@rexf rexf commented Feb 12, 2025

update_service_to_support_adding_endpoint_after_construction

@rexf
Copy link
Contributor Author

rexf commented Feb 12, 2025

for this discussion
#1029

@rexf
Copy link
Contributor Author

rexf commented Feb 13, 2025

@scottf any comments?

Copy link
Contributor

@scottf scottf left a comment

Choose a reason for hiding this comment

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

please fix all formatting not part of code change.

private static final String INTEGER_RE = "(-?\\d+)";
private static final String STRING_RE = "\"(.+?)\"";
private static final String BOOLEAN_RE = "(true|false)";
private static final String INTEGER_RE = "(-?\\d+)";
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any way you can make functional changes without changing the formatting. This makes unnecessary changes to the code that require review, and it's changes the formatting that have been in place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok - reverting the format change by intellij autoformat.

sb.append("\n}");
return sb.toString().replaceAll(",", ",\n ");
}

/**
* Appends a json field to a string builder.
* @param sb string builder
*
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a non-essential change also and there are many of these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

k

@@ -461,30 +497,25 @@ public static String getFormatted(Object o) {
indent = indent(++level);
indentNext = true;
lastWasClose = false;
}
else if (c == '}') {
} else if (c == '}') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely not. I prefer else on it's own line. It's everywhere in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

k

? dispatcher.subscribe(se.getSubject(), this::onMessage)
: dispatcher.subscribe(se.getSubject(), qGroup, this::onMessage);
started = DateTimeUtils.gmtNow();
if (null == sub) {
Copy link
Contributor

Choose a reason for hiding this comment

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

please change to sub == null to be consistent with everywhere else in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

k

src/main/java/io/nats/service/Service.java Show resolved Hide resolved
@rexf rexf requested a review from scottf February 13, 2025 16:56
@rexf
Copy link
Contributor Author

rexf commented Feb 13, 2025

thanks @scottf, i hope all concerns have been addressed.

Copy link
Contributor

@scottf scottf left a comment

Choose a reason for hiding this comment

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

Awaiting response from CLI developer before proceeding

@scottf
Copy link
Contributor

scottf commented Feb 14, 2025

@rexf The CLI developer suggested to change the Java client since the CLI is already out in the wild. I'll finish the review soon.
Can you also change the StatsResponse to add the empty list. Might as well be consistent.

@rexf
Copy link
Contributor Author

rexf commented Feb 15, 2025

@scottf done.

Copy link
Contributor

@scottf scottf left a comment

Choose a reason for hiding this comment

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

LGTM

@rexf rexf force-pushed the feat/update_service_to_support_adding_dynamic_endpoint_after_service_construction branch from 47466e7 to 56edd32 Compare February 16, 2025 22:47
@rexf
Copy link
Contributor Author

rexf commented Feb 17, 2025

@scottf all commit signed. thanks for the help.

@scottf scottf merged commit dabcba8 into nats-io:main Feb 17, 2025
1 check passed
@scottf scottf changed the title feature: update_service_to_support_adding_endpoint_after_construction Feature: Support adding service endpoint after construction Feb 17, 2025
@rexf rexf deleted the feat/update_service_to_support_adding_dynamic_endpoint_after_service_construction branch February 17, 2025 14:13
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.

2 participants