Added type checking and update types (LSP-3.15)

[danixeee]

Description (e.g. "Related to ...", etc.)

• Remove python 3.5 support

• New directory structure for types

• Added new types according to LSP-3.15 specs

• Added type checking (pydantic, typegueard) for:

  • Feature options
  • Feature params
  • Feature return type

• Store client/server capabilities

• Added a lot of tests

• test manually with projects that are using pygls to make sure that everything is still working

Related to #59, #137

Closes #81.

Code review checklist (for code reviewer to complete)

• Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
• Title summarizes what is changing
• Commit messages are meaningful (see this for details)
• Tests have been included and/or updated, as appropriate
• Docstrings have been included and/or updated, as appropriate
• Standalone docs have been updated accordingly
• CONTRIBUTORS.md was updated, as appropriate
• Changelog has been updated, as needed (see CHANGELOG.md)

[perrinjerome]

Hello @danixeee , this is great !

I left a few inline comments for minor things.

I also tried to update https://github.com/perrinjerome/vscode-zc-buildout to use this branch. Generally speaking, everything works 👍 here's what I did: first I updated imports, pygls.types is now pygls.lsp.types, pygls.features is now pygls.lsp.methods. Then I updated all places where lsp objects where created directly to use keywords arguments in constructors.
I had to change several

Position(1, 2)

into

Position(line=1, character=2)

because the former is no longer valid. I guess it's OK to break compatibility like this, but in my opinion something should be mentioned in changelog.

I also had to change a lot of

Range(Position(1, 2), Position(3, 4))

into

Range(start=Position(line=1, character=2), end=Position(line=3, character=4))

and at some point I made a mistake, I did something like:

Position(start=1, stop=2)

I was disappointed, because this project have a good type coverage, but that error was not reported by mypy. It was also no error at runtime. I looked a bit at pydantic, the models can be configured with extra = "forbid" to disallow this. Unfortunately, if we add extra = "forbid" in the base model here:

pygls/pygls/lsp/types/basic_structures.py

Lines 46 to 49 in a5ac274

	class Model(BaseModel):
	class Config:
	alias_generator = snake_to_camel
	allow_population_by_field_name = True

this cause errors during initialization. I did not investigate the error, the message was:

pydantic.error_wrappers.ValidationError: 2 validation errors for InitializeParams
capabilities -> textDocument -> completion -> completionItem -> commitCharactersSupport
  extra fields not permitted (type=value_error.extra)
capabilities -> textDocument -> completion -> completionItem -> preselectSupport
  extra fields not permitted (type=value_error.extra)

raised here:

pygls/pygls/protocol.py

Line 144 in a5ac274

except ValueError:

... so it seems we can not use extra='forbid' in config. Maybe I missed something here, but I guess if lsp client sends extra properties it should not be an error and these properties should be ignored.

Anyway, good news is that there is a mypy plugin that comes with pydantic, after adding the following mypy config:

[mypy]
plugins = pydantic.mypy

[pydantic-mypy]
init_forbid_extra = True

such errors are reported at type checking time:

error: Unexpected keyword argument "start" for "Position"
error: Unexpected keyword argument "end" for "Position"

PS: Maybe things would have been better if Position did not have default values for line and characters ?

pygls/pygls/lsp/types/basic_structures.py

Lines 87 to 88 in a5ac274

	line: int = 0
	character: int = 0

there's no such default values in https://microsoft.github.io/language-server-protocol/specification#position

[danixeee]

Great catches! I think I have addressed everything you found.

... so it seems we can not use extra='forbid' in config. Maybe I missed something here, but I guess if lsp client sends extra properties it should not be an error and these properties should be ignored.

There were additional fields that were wrong and causing validation to fail. Do you think I should keep extra = "forbid" in the base model class?

Maybe things would have been better if Position did not have default values for line and characters?

Not sure if this would help you to detect an issue easier, I will keep default values as they are saying that values are zero-based.

@perrinjerome, @dimbleby Could you check everything again and left comments if everything is ok now?

[dimbleby]

Latest changes haven't broken anything for me!

I'd be inclined to agree with @perrinjerome that Position shouldn't have defaults: I don't know that defaulting it to (0, 0) will help anyone remember that it's zero-based, whereas it definitely provides an opportunity to go wrong. But I don't feel very strongly about it.

[danixeee]

@dimbleby Ok, I think you two are right, we should just follow exact specs...

Do you have any thought about adding extra = "forbid" to the base model?

[dimbleby]

Again I agreewith @perrinjerome: "forbid" feels dangerous for back-compatibility reasons: if and when they extend the spec so that clients can provide additional fields, it would be a shame for servers to stop working completely for those clients.

(probably such things should be negotiated by capabilities first, but I bet not everyone does that).

[danixeee]

I think this is ready to be merged. @renatav If you want, you can do a final review, or just approve it.

Regarding the release, I think it's important to investigate and fix #118 and #130 before publishing a new version.

[renatav]

The newest changes look good to me. Merge if no further work should be done based on the other feedback

[dimbleby]

Regarding the release, I think it's important to investigate and fix #118 and #130 before publishing a new version.

IMO it would be a shame to block releasing this on those two. They both are longstanding and as I understand them are quite independent of this rework. You can always go 0.10.1 after 0.10.0.

Of course if you think that it'll be easy to deal with those two without delaying release enough to care about: then great, more fixes is good!