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

Add dmypy support via mypy.api.run_dmypy #1

Merged
merged 2 commits into from
May 20, 2021

Conversation

asford
Copy link

@asford asford commented Mar 13, 2021

palantir/python-language-server#391

Update plugin flow for non-live-mode dmypy invocation via mypy.api.run_dmypy.

Minor fix to update detect .mypy.ini as well as mypy.ini:
https://mypy.readthedocs.io/en/stable/config_file.html

@asford
Copy link
Author

asford commented May 18, 2021

@Richardk2n Friendly ping, I'm not sure if you'd like to fold this in along with #2 but it'd be great to include this if possible.

I've fixed up the lingering 3.6 syntax failures.

@asford
Copy link
Author

asford commented May 18, 2021

Referenced in tomv564#51

@Richardk2n
Copy link
Member

Once #2 is successfully merged (probably in a few minutes), I will look at this (maybe ask some questions) and then probably merge it as well.

Copy link

@haplo haplo left a comment

Choose a reason for hiding this comment

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

Just minor issues with string interpolation in logging statements and references to python-language-server instead of python-lsp-server.

README.rst Outdated Show resolved Hide resolved
mypy_ls/plugin.py Outdated Show resolved Hide resolved
mypy_ls/plugin.py Outdated Show resolved Hide resolved
mypy_ls/plugin.py Outdated Show resolved Hide resolved
mypy_ls/plugin.py Outdated Show resolved Hide resolved
@Richardk2n
Copy link
Member

Richardk2n commented May 18, 2021

@asford Should the daemon be closed for example atexit ?

Copy link
Author

@asford asford left a comment

Choose a reason for hiding this comment

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

Updates from review.

README.rst Outdated Show resolved Hide resolved
mypy_ls/plugin.py Outdated Show resolved Hide resolved
mypy_ls/plugin.py Outdated Show resolved Hide resolved
mypy_ls/plugin.py Outdated Show resolved Hide resolved
mypy_ls/plugin.py Outdated Show resolved Hide resolved
mypy_ls/plugin.py Outdated Show resolved Hide resolved
mypy_ls/plugin.py Outdated Show resolved Hide resolved
mypy_ls/plugin.py Outdated Show resolved Hide resolved
@asford
Copy link
Author

asford commented May 18, 2021

@asford Should the daemon be closed for example atexit ?

I don't think the daemon should be closed, as the daemon can persist expensive cached state between editor sessions.
For example, I use vim-lsp and end-up reusing the dmypy daemon as I hop between vim sessions.
In some codebases, where dmypy is ...extremely useful... starting the daemon for a first run is extremely expensive.
Most users are probably just calling dmypy run from the command line, which doesn't stop the server after the run.

@asford asford requested a review from Richardk2n May 18, 2021 20:33
mypy_ls/plugin.py Outdated Show resolved Hide resolved
mypy_ls/plugin.py Outdated Show resolved Hide resolved
@haplo
Copy link

haplo commented May 19, 2021

The codebase is now formatted with black after #4 was merged. @asford you can pull from master, ignore all changes to plugin.py and then run black . to blacken your changes.

@Richardk2n
Copy link
Member

@asford please note that 90a37e1 contains how I would like to have this merged. I put it in a commit to make sure it does not get lost when you pull/merge from master.

Copy link
Author

@asford asford left a comment

Choose a reason for hiding this comment

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

f-string updates

mypy_ls/plugin.py Outdated Show resolved Hide resolved
mypy_ls/plugin.py Outdated Show resolved Hide resolved
mypy_ls/plugin.py Outdated Show resolved Hide resolved
mypy_ls/plugin.py Outdated Show resolved Hide resolved
palantir/python-language-server#391

Update plugin flow for non-live-mode dmypy invocation via run_dmypy.

Minor fix to update detect `.mypy.ini` as well as `mypy.ini`.
@asford
Copy link
Author

asford commented May 19, 2021

Roger, thanks for the through review.
I've squash-rebased on the most-recent upstream, so this should be a minimal mergeable diff.

Copy link
Member

@Richardk2n Richardk2n left a comment

Choose a reason for hiding this comment

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

Looks good

@Richardk2n Richardk2n merged commit 6b5b5fb into python-lsp:master May 20, 2021
@rchl
Copy link

rchl commented Jul 2, 2021

I've noticed that using dmypy through pylsp creates a .dmypy.json file in every project. Ideally that wouldn't happen. Is it possible to create that file in a temp directory?

@Richardk2n
Copy link
Member

I've noticed that using dmypy through pylsp creates a .dmypy.json file in every project. Ideally that wouldn't happen. Is it possible to create that file in a temp directory?

mypy also creates a .mypy_cache folder, which seems to not be a problem?
Why is .dmypy.json a problem?

@rchl
Copy link

rchl commented Jul 3, 2021

I guess I'm used to .mypy_cache being git-ignored while .dmypy.json is new to me and from the naming looks like a configuration file rather than something that should be ignored.

From the documentation it looks like it's in fact a temporary file that could be ignored but in that case can't we actually define its location to be .mypy_cache/.dmypy.json? I think that would be nice if it worked.

But in the worse case, git-ignoring .dmypy.json is an acceptable solution of course. But it would require updating .gitignore file in many projects.

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.

4 participants