-
Notifications
You must be signed in to change notification settings - Fork 158
Adds 'instructions' string to the Server and propagates to the InitializeResult #290
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
Changes from all commits
f5f24fa
574c435
0d8253e
c0249a5
b8a5945
6a95a59
4b7674f
1a714d7
74aeaff
3e682ec
083ff48
b05a196
99555d2
8600054
7a02d56
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,8 +53,28 @@ public class ServerOptions(public val capabilities: ServerCapabilities, enforceS | |
* | ||
* @param serverInfo Information about this server implementation (name, version). | ||
* @param options Configuration options for the server. | ||
* @param instructionsProvider Optional provider for instructions from the server to the client about how to use | ||
* this server. The provider is called each time a new session is started to support dynamic instructions. | ||
*/ | ||
public open class Server(private val serverInfo: Implementation, private val options: ServerOptions) { | ||
|
||
public open class Server( | ||
protected val serverInfo: Implementation, | ||
protected val options: ServerOptions, | ||
protected val instructionsProvider: (() -> String)? = null, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this implemented as a lambda and not just a string? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is allowing the instructions to potentially be dynamically loaded from a database or string server. With the change to the ServerSession, the Server seems to take more of a Singleton scope vs a Server per stream. The idea is that the Server has a provider method for the string which it evaluates as a string value for the ServerSession.
I went ahead and added an alternative constructor that takes a static string |
||
) { | ||
/** | ||
* Alternative constructor that provides the instructions directly as a string. | ||
* | ||
* @param serverInfo Information about this server implementation (name, version). | ||
* @param options Configuration options for the server. | ||
* @param instructions Instructions from the server to the client about how to use this server. | ||
*/ | ||
public constructor( | ||
serverInfo: Implementation, | ||
options: ServerOptions, | ||
instructions: String, | ||
) : this(serverInfo, options, { instructions }) | ||
|
||
private val sessions = atomic(persistentListOf<ServerSession>()) | ||
|
||
@Suppress("ktlint:standard:backing-property-naming") | ||
|
@@ -90,7 +110,7 @@ public open class Server(private val serverInfo: Implementation, private val opt | |
* @return The initialized and connected server session. | ||
*/ | ||
public suspend fun connect(transport: Transport): ServerSession { | ||
val session = ServerSession(serverInfo, options) | ||
val session = ServerSession(serverInfo, options, instructionsProvider?.invoke()) | ||
|
||
// Internal handlers for tools | ||
if (options.capabilities.tools != null) { | ||
|
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.
nit: I think we should introduce some serialization helper for tests like this, like here
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.
This would probably be applicable to all the tests in this file, so I would prefer to leave it to a different PR if that's ok
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.
I agree with both statements