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

Support 3.13 #79

Merged
merged 14 commits into from
Sep 23, 2018
Merged

Support 3.13 #79

merged 14 commits into from
Sep 23, 2018

Conversation

kjeremy
Copy link
Contributor

@kjeremy kjeremy commented Sep 19, 2018

This should bump us up to 3.13. It's probably best to review each commit separately.

Supercedes #78
Fixes #76

Copy link
Member

@Marwes Marwes left a comment

Choose a reason for hiding this comment

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

<3 That's a lot of changes! I went through and double checked it fairly throughly and found only one thing that is wrong (or rather it should be updated to support the new features).

src/lib.rs Outdated
* `workspace.workspaceEdit.documentChanges` client capability.
*
* If a client neither supports `documentChanges` nor `workspace.workspaceEdit.resourceOperations` then
* only plain `TextEdit`s using the `changes` property are supported.
*/
#[serde(skip_serializing_if = "Option::is_none")]
pub document_changes: Option<Vec<TextDocumentEdit>>,
Copy link
Member

Choose a reason for hiding this comment

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

s/TextDocumentEdit/DocumentChanges?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I'm reading it right the Vec needs to be removed too.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, since DocumentChanges contains a Vec

src/lib.rs Outdated
}

#[derive(Debug, Eq, PartialEq, Clone, Deserialize, Serialize)]
#[serde(untagged)]
Copy link
Member

Choose a reason for hiding this comment

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

I think you could encode this with https://serde.rs/container-attrs.html#serdetag--type + https://serde.rs/variant-attrs.html#serdeother and then the kind fields could probably be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would you deal with the different options like CreateFileOptions etc.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh wait I think I see...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure this combination will work. #[serde(other)] only works on unit variants.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I THINK inserting a shim gives us the correct results:

#[derive(Debug, Eq, PartialEq, Clone, Deserialize, Serialize)]
#[serde(untagged)]
pub enum DocumentChanges {
    Edits(Vec<TextDocumentEdit>),
    Operations(Vec<DocumentChangeOperation>),
}

#[derive(Debug, Eq, PartialEq, Clone, Deserialize, Serialize)]
#[serde(untagged, rename_all="lowercase" )]
pub enum DocumentChangeOperation {
    Op(ResourceOp), // <------- In an ideal world `ResourceOp` wouldn't be needed
    Edit(TextDocumentEdit),
}

#[derive(Debug, Eq, PartialEq, Clone, Deserialize, Serialize)]
#[serde(tag = "kind", rename_all="lowercase" )]
pub enum ResourceOp {
    Create(CreateFile),
    Rename(RenameFile),
    Delete(DeleteFile),
}

This looks right according to my test except that the TextDocumentEdit's fields are swapped so the string equality fails.

src/lib.rs Outdated
@@ -2337,6 +2687,8 @@ pub struct ApplyWorkspaceEditResponse {
pub applied: bool,
}

pub type CodeActionKind = String;
Copy link
Member

Choose a reason for hiding this comment

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

There are some predefined CodeActionKind values that could be added and exported (search for export namespace CodeActionKind)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw that but wasn't sure if those values were open ended or not (or how best to encode that?)

Copy link
Member

Choose a reason for hiding this comment

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

Could just add a few &str constants to make the available.

@Marwes Marwes mentioned this pull request Sep 21, 2018
@kjeremy
Copy link
Contributor Author

kjeremy commented Sep 23, 2018

Okay I've addressed your comments.

Allows interoperability with vscode-language-client 5.1.0
@kjeremy
Copy link
Contributor Author

kjeremy commented Sep 23, 2018

Should be good now. With bf12571 we can now run against a language client build with the latest vscode-language-client 5.1.0

@Marwes Marwes merged commit 4340c36 into gluon-lang:master Sep 23, 2018
@Marwes
Copy link
Member

Marwes commented Sep 23, 2018

Thanks again! Released as 0.51

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