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

feat: add support for pyls-mypy #232

Merged
merged 10 commits into from
Mar 23, 2021
Merged

feat: add support for pyls-mypy #232

merged 10 commits into from
Mar 23, 2021

Conversation

tillhainbach
Copy link
Contributor

No description provided.

lib/main.js Outdated Show resolved Hide resolved
@aminya aminya changed the title Add support for pyls-mypy feat: add support for pyls-mypy Dec 12, 2020
Obviously, it should have been git+https: ... from the start...
@aminya
Copy link
Member

aminya commented Feb 6, 2021

What is the state of this? Is it ready for merging?

@aminya
Copy link
Member

aminya commented Mar 14, 2021

@tillhainbach Let me know when this is ready. I can squash the commits to fix the commitlint error.

@tillhainbach
Copy link
Contributor Author

Hey, from my point of view the state is as follows:

1. Merge Conflict and CI Tests:

I merged my branch with the master branch and resolved the merge conflicts there. However, now there is a new commit-linter in the CI, so the linter-test fails on the old commit messages. I think I could go back and reword all commit messages to the linter's standard but that would be a little tedious.

2. Pyls-Mypy:

The owner of the pyls-mypy repo seems to be "off the grid" and the pyls-group is going to take over maintenance for pyls-mpypy (tomv564/pyls-mypy#44). But there hasn't been any progress since last September, as far as I can tell.

@tillhainbach
Copy link
Contributor Author

tillhainbach commented Mar 14, 2021

@aminya fixing the commitlint errors would be nice.

@aminya
Copy link
Member

aminya commented Mar 15, 2021

So, how can I test if this is working? Does this automatically work in combination with python-language-server?

My test, run a pnpm i, install pyls-mypy, and make a python file to test the language server

import os 

os.chdir()

@lgeiger Maybe you can clarify this here.

@tillhainbach
Copy link
Contributor Author

tillhainbach commented Mar 15, 2021

If Mypy is enabled in the ide-python settings and pyls-mypy is installed it should work out-of-the-box.

I'm not sure how you catch if an error is thrown, but pyls-mypy should throw an error if you do something like this:

an_int: int = "1"

@aminya
Copy link
Member

aminya commented Mar 21, 2021

Unfortunately, this doesn't seem to work. The error isn't transmitted to Atom. Is there an additional spawning needed for this?

@tillhainbach
Copy link
Contributor Author

@aminya I just remembered that I have set the default configuration for mypy to "live mode -> false" (because of caveats here). I tested it myself like this:

# clone repo and checkout new feat branch
gh repo clone tillhainbach/ide-python
cd ide-python
git checkout add-pyls-mypy-support


# link into dev/packages
apm develop ide-python

# install package
apm install

# create test dir with python virtual env
mkdir test_mypy
cd test_mypy
python -m venv venv
. ./venv/bin/activate

# install pyls and pyls-mypy
python -m pip install 'python-language-server[all]'
python -m pip install git+https://github.com/tomv564/pyls-mypy.git

# create python file with type error
echo 'an_int: int = "1"' >> test_mypy_error.py

atom -d .
# no error
# file -> save --> mypy error

@aminya
Copy link
Member

aminya commented Mar 22, 2021

Shouldn't this give an error? I am confused.

@tillhainbach
Copy link
Contributor Author

Could you give me a guide, how you would go about testing this/intercepting atom errors? Then I could try it out locally and report back. Is there something like a docker image with a clean atom installation or something?

For clarification:
In the default settings Mypy is only triggered after one saves the file from within atom.

package.json Outdated Show resolved Hide resolved
Set default live-mode to pyls-mypy defaults.

Co-authored-by: Amin Yahyaabadi <[email protected]>
@tillhainbach
Copy link
Contributor Author

Should there be a note something like: "turning live-mode off may result in mypy not working on some machines"? Because apparently this setting seems to be a pit picky?

@aminya
Copy link
Member

aminya commented Mar 22, 2021

Should there be a note something like: "turning live-mode off may result in mypy not working on some machines"? Because apparently this setting seems to be a pit picky?

Yes, adding a note in the config description would be useful

Copy link
Member

@aminya aminya left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

@aminya aminya merged commit fb48958 into atom-community:master Mar 23, 2021
@github-actions
Copy link

🎉 This PR is included in version 1.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@aminya aminya mentioned this pull request Jun 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants