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

Add an optional exitCode to ExecStatusEvent so clients can use it #3991

Merged
merged 7 commits into from Mar 15, 2018

Conversation

Projects
None yet
2 participants
@dwijnand
Copy link
Member

commented Mar 6, 2018

No description provided.

@dwijnand dwijnand added the in progress label Mar 6, 2018

@dwijnand dwijnand added this to the 1.1.2 milestone Mar 6, 2018

@dwijnand dwijnand requested a review from eed3si9n Mar 6, 2018

StandardMain.exchange publishEventMessage ExecStatusEvent(
"Done",
channelName,
exec.execId,
newState.remainingCommands.toVector map (_.commandLine))
newState.remainingCommands.toVector map (_.commandLine),
exitCode,

This comment has been minimized.

Copy link
@eed3si9n

eed3si9n Mar 6, 2018

Member

There's also successful event too right?
This needs some rethinking.

This comment has been minimized.

Copy link
@dwijnand

dwijnand Mar 6, 2018

Author Member

you're right.. last second change. will fix tomorrow

@dwijnand dwijnand force-pushed the dwijnand:ExecStatusEvent-exitCode branch from 20559ae to 902786e Mar 7, 2018

@dwijnand dwijnand force-pushed the dwijnand:ExecStatusEvent-exitCode branch from 902786e to 8227cef Mar 8, 2018

@eed3si9n

This comment has been minimized.

Copy link
Member

commented Mar 10, 2018

LSP already has the concept of Response and Error, so I'd say we should handle these in a more idiomatic way.
see also #3863

@eed3si9n
Copy link
Member

left a comment

I think we should fold this into #3863

@dwijnand dwijnand force-pushed the dwijnand:ExecStatusEvent-exitCode branch 2 times, most recently from 62dc2da to b952cbe Mar 12, 2018

@dwijnand dwijnand requested a review from eed3si9n Mar 13, 2018

@eed3si9n
Copy link
Member

left a comment

Getting closer.

}

private def determineExitCode(state: State, newState: State): Option[Int] = {
sealed trait ExitCode

This comment has been minimized.

Copy link
@dwijnand

dwijnand Mar 13, 2018

Author Member

what are you requesting is changed?

This comment has been minimized.

Copy link
@eed3si9n

eed3si9n Mar 13, 2018

Member

You're currently making up 0 or 1 based numbering that gets sent to LSP client correct?
http://www.jsonrpc.org/specification determines the number you have to use.

This comment has been minimized.

Copy link
@dwijnand

dwijnand Mar 13, 2018

Author Member

it says:

The remainder of the space is available for application defined errors.

I'm using 1 when there is some unknown error that was handled by onFailure, that's it.

This comment has been minimized.

Copy link
@eed3si9n

eed3si9n Mar 14, 2018

Member

VS Code uses -32800 for cancellation. Maybe it's better to extend to some large negative integers following the convention, like -33000.

This comment has been minimized.

Copy link
@dwijnand

dwijnand Mar 14, 2018

Author Member

why is -33000 better than 1 for "unknown error"?

This comment has been minimized.

Copy link
@eed3si9n

eed3si9n Mar 15, 2018

Member

From the point of view of the LSP client (let's say Vim or something) they need to handle various error events. I want to fit into the ecosystem harmoniously.

dwijnand added some commits Mar 14, 2018

Rename ErrorCodes' UnknownErrorCode to UnknownServerError
Allows for a non-server-specific unknown error code to be defined.
Introduce ErrorCodes.UnknownError
Defined in the application defined errors range.

@dwijnand dwijnand force-pushed the dwijnand:ExecStatusEvent-exitCode branch from b952cbe to 32af56e Mar 14, 2018

@dwijnand dwijnand requested a review from eed3si9n Mar 14, 2018

@dwijnand dwijnand force-pushed the dwijnand:ExecStatusEvent-exitCode branch from 32af56e to 98332c0 Mar 14, 2018

@dwijnand

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2018

jfyi, this is green now @eed3si9n

@eed3si9n eed3si9n merged commit 4efce7e into sbt:1.1.x Mar 15, 2018

3 checks passed

Codacy/PR Quality Review Good work! A positive pull request.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@eed3si9n eed3si9n removed the in progress label Mar 15, 2018

@dwijnand dwijnand deleted the dwijnand:ExecStatusEvent-exitCode branch Mar 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.