-
Notifications
You must be signed in to change notification settings - Fork 33
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 typing to public methods #52
Conversation
Hi, |
I'm revertng this commit for now because it seems to break mypy: https://github.com/yeger00/pylspclient/actions/runs/8457766973/job/23170487640 Please review and send it again. (I added mypy to the main pipeline as well, you can run make lint to test it) Thanks |
ahh ok |
I see that you got both ruff and mypy in flow. Aren't most rules on mypy covered by ruff? |
I'm not that familiar but I don't see it mentioned in the Ruff README |
@@ -41,7 +42,7 @@ def __add_header(json_string): | |||
return JSON_RPC_REQ_FORMAT.format(json_string_len=len(json_string), json_string=json_string) | |||
|
|||
|
|||
def send_request(self, message): | |||
def send_request(self, message: any) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be typing.Any
rootPath: Optional[str] = None, | ||
rootUri: Optional[str] = None, | ||
initializationOptions: any = None, | ||
capabilities: dict = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
capabilities: OPtional[dict] = None,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is actually a required parameter, but some are optional parameters above this. This should ideally be reordered, but that would be a breaking change.
OK will add mypy to the flow. I used to think mypy was covered by ruff. Will send a seperate PR with the changes. |
Add typing so we don't need to break our head wondering what to pass.
Also fix initialize to bypass having to pass in explicit null for deprecated parameters.