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

Send token with WorkDoneProgress #732

Merged
merged 1 commit into from
Apr 20, 2023
Merged

Send token with WorkDoneProgress #732

merged 1 commit into from
Apr 20, 2023

Conversation

JCWasmx86
Copy link
Contributor

This fixes the $/progress notification, as it requires that the ProgressToken is sent with the value.

swift test works, theoretically it should probably be called ProgressParams, but it wouldn't reflect that it is a notification, so I just changed the enum to WorkDoneProgressType

/// The progress data.
public var value: WorkDoneProgressType

public init(token: ProgressToken, value: WorkDoneProgressType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this initializer used anywhere outside of the tests? I don't see any other references to it in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No due to #577. I use your type definitions in another language server and want to use $/progress => I implement it and send it upstream

@MaxDesiatov
Copy link
Contributor

@swift-ci test

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Thank you. I must have missed that when going through all the struct definitions in #669. I’ve got one naming suggestion, otherwise this looks good to me 👍🏽

@MaxDesiatov
Copy link
Contributor

@swift-ci test

@ahoppen
Copy link
Member

ahoppen commented Apr 20, 2023

@swift-ci Please test Windows

@ahoppen
Copy link
Member

ahoppen commented Apr 20, 2023

Thank you. I don’t know how much you care but if you’d like this to be in the Swift 5.9 release, you will need to create a separate PR for the release/5.9 branch.

@JCWasmx86
Copy link
Contributor Author

Personally it's not that important to have it at any tag, as I just use the main branch from this repo

@ahoppen
Copy link
Member

ahoppen commented Apr 20, 2023

OK, in that case there’s no reason to cherry-pick it.

@ahoppen ahoppen merged commit 2f24c3c into swiftlang:main Apr 20, 2023
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.

3 participants