Skip to content

Conversation

dlax
Copy link

@dlax dlax commented Aug 27, 2025

A few commits to make mypy happy in the code base.

@dlax dlax marked this pull request as ready for review August 27, 2025 08:04
@Richardk2n
Copy link
Member

Richardk2n commented Aug 27, 2025

Thank you.
The reason for List and Dict was because you are not able to use the builtins in older python versions.
Do you happen to know, since when this is possible? (To make sure we still support the versions we claim to support)

Did you figure out, why the call overload is complaining?
I never did. It looks fine to me.

dlax added 2 commits August 27, 2025 12:10
Since we target Python 3.9, typing.Dict and typing.List are deprecated
and builtins can be used instead.
@dlax
Copy link
Author

dlax commented Aug 27, 2025

The reason for List and Dict was because you are not able to use the builtins in older python versions. Do you happen to know, since when this is possible? (To make sure we still support the versions we claim to support)

This is possible since Python 3.9, so fine for us.

Did you figure out, why the call overload is complaining? I never did.

This is because of the **windows_flag; but in fact, I found a better way to fix those, see last commit.

@Richardk2n
Copy link
Member

Ok good.

Can you elaborate a bit?
I am fine with this solution if it works, but am not quite sure why it does.
Also, the #type ignore in that line should be removable now?
The tests also have these, so maybe use the typed dict there as well.

@dlax
Copy link
Author

dlax commented Aug 27, 2025

Can you elaborate a bit? I am fine with this solution if it works, but am not quite sure why it does.

The previous type hint for windows_flag: dict[str, int] resulted in a subprocess.run(**windows_flag: int) call (from typing pov) but the function does not accept arbitraty **kwargs: int. By changing to a TypedDict, we can keep the **windows_flag syntax but now the call is subprocess.run(creationflags: int), which is allowed.

Also, the #type ignore in that line should be removable now?

No, it's still needed because, on linux, subprocess has no CREATE_NO_WINDOW attribute; I added an error code for that.

The tests also have these, so maybe use the typed dict there as well.

I changed the test to reuse the value from plugin module.

@Richardk2n
Copy link
Member

Ok, thx for the explanation.
Can you please add a comment to the line about the ignoring being necessary even so this never can produce an error?

It seems like you did not run the black formatter.

@dlax
Copy link
Author

dlax commented Aug 27, 2025

Done

@dlax
Copy link
Author

dlax commented Aug 27, 2025

Fixed the flake8 error, sorry about that.

@Richardk2n
Copy link
Member

line 73 still is too long?

dlax added 3 commits August 27, 2025 15:18
This 'type ignore' is because mypy will raise an error on Linux as the
subprocess module does not have a CREATE_NO_WINDOW attribute defined.
@dlax
Copy link
Author

dlax commented Aug 27, 2025

line 73 still is too long?

fixed...

@Richardk2n Richardk2n self-requested a review August 27, 2025 13:22
@Richardk2n Richardk2n merged commit 7cde613 into python-lsp:master Aug 27, 2025
11 checks passed
@Richardk2n
Copy link
Member

Thank you for your contribution.

@dlax dlax deleted the typing branch August 27, 2025 14:24
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