openlawlibrary / pygls

A pythonic generic language server
https://pygls.readthedocs.io/en/latest/
Apache License 2.0
563 stars 102 forks source link

Add more capability unit tests and enable coverage reporting #380

Closed alcarney closed 11 months ago

alcarney commented 11 months ago

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

This PR attempts to address issues around computing server capabilities raised in #377

Also

Code review checklist (for code reviewer to complete)

dimbleby commented 11 months ago

(I don't care either way about the import changes: except that that putting such things into the same commit as the actual substantial change that you are making is unhelpful for reviewers!)

I'd suggest that the default value for _provider_options() should be None rather than True: and that callers who want a True default should have to say so. It sort of doesn't matter, you will feel free to ignore me, clearly it's possible to write correct code either way. But I reckon that this is what allowed the confusion that caused the bug to be written in the first place: None is the conventional default on lots of APIs and it's natural for developers to expectt the same here.

alcarney commented 11 months ago

(I don't care either way about the import changes: except that that putting such things into the same commit as the actual substantial change that you are making is unhelpful for reviewers!)

Apologies! I probably should've marked this as draft before, as I wasn't necessarily expecting a review :sweat_smile:

tombh commented 11 months ago

I approved the PR, but @dimbleby, please chime in as well if anything stands out.

Coverage is a nice touch! Thoughts for another time: Should we set it up so that a PR is refused if coverage goes down? We could add https://coveralls.io integration for nice graphs and stuff?

alcarney commented 11 months ago

Should we set it up so that a PR is refused if coverage goes down? We could add https://coveralls.io/ integration for nice graphs and stuff?

Up to you.

Another option is to copy the attrs project and have the report show up in the workflow summary. It looks like it would be fairly easy to setup

tombh commented 11 months ago

Yeah having the summary in the workflow like that is nice. I'll see if I can add that soon.