-
Notifications
You must be signed in to change notification settings - Fork 676
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
Clear nullability warnings in server/omnisharp.ts #5199
Clear nullability warnings in server/omnisharp.ts #5199
Conversation
Encapsulates variables that only have meaning during certain states.
private _makeRequest(request: Request) { | ||
private _makeRequest(request: Request): number { | ||
if (this._state.status !== ServerState.Started) { | ||
throw new Error("Tried to make a request when the OmniSharp server wasn't running"); |
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.
Should this post to the EventStream instead?
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 could see it emitting an event. OmnisharpFailure
isn't what we want because it focuses the OmniSharp Log in the Output pane. It likely needs a new Event type which will be logged by the OmnisharpDebugModeLoggerObserver
. This could certainly come as a follow up.
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.
Okay, I looked into what it would take to make this emit an event instead. The event emitting itself is easy; the problem is that _makeRequest
needs to return an ID that's used by RequestQueue: https://github.com/OmniSharp/omnisharp-vscode/blob/f1f38b774243526a798a16922d3b8c2f2008a84b/src/omnisharp/requestQueue.ts#L95-L99
So, two options:
- Keep the
throw
to stop execution. - Change
_makeRequest
to returnnumber | undefined
, emit the event, and don't add the request to the queue if it's undefined.
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!
Mostly, this adds a mini state machine to server.ts that controls which variables can be accessed.
I believe there's the following behavior changes to watch out for:
_makeRequest
now errors out if a request is sent when the server isn't running. (This would throw an uncaught exception anyway because ofthis._serverProcess
being undefined, now it's just explicit.)stop
now short-circuits if the server is already stopped.autoStart
now calls_start
directly rather than going throughrestart
. Given how it's called autoStart, I feel like this makes sense.Potential future improvements:
o.restart
currently tries to restart by implicitly using the server's last-known launch target. However, it could be possible if we...o.restart
to target.My naming scheme for ServerState and State is terrible. Please advise on better names for them :).