Skip to content

pyright setup #374

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

Closed
schwehr opened this issue May 31, 2021 · 3 comments
Closed

pyright setup #374

schwehr opened this issue May 31, 2021 · 3 comments

Comments

@schwehr
Copy link
Collaborator

schwehr commented May 31, 2021

pyright is in scripts/pyright, but it's not elsewhere. It should be in requirements-dev.txt, yes? But it doesn't seem to be in pypi?

And while looking at the pyright script, I notice that it's a bash script that uses which. Better to use the bash-builtin of type -p pyright

find . -type f | egrep -v '.git|ve/|mypy' | xargs grep pyright | egrep -i -v 'copyright'
grep: ./docs/_build/doctrees/environment.pickle: binary file matches
./scripts/pyright:PATH_TO_PYRIGHT=`which pyright`
./scripts/pyright:# If we can't find pyright, install it.
./scripts/pyright:echo "Checking pyright exists..."
./scripts/pyright:    echo "...installing pyright"
./scripts/pyright:    ${SUDO} npm install -g pyright
./scripts/pyright:    echo "Checking pyright version..."
./scripts/pyright:    CURRENT=`pyright --version | cut -d' ' -f2`
./scripts/pyright:    REMOTE=`npm info pyright version`
./scripts/pyright:        echo "...new version of pyright found, upgrading."
./scripts/pyright:        ${SUDO} npm upgrade -g pyright
./scripts/pyright:pyright "$@"
./scripts/test:        scripts/pyright pystac tests
@duckontheweb
Copy link
Contributor

pyright is an npm package, so we wouldn't be able to put it in requirements-dev.txt. ./scripts/pyright (which gets run by ./scripts/test) automatically installs it if it is not installed already, so it doesn't seem like there's a need to include separate installation instructions anywhere.

However, we may want to include something in the Contributing docs to let people know that the script will install pyright and that they will need to have Node.js/npm installed.

Stepping back, I'm wondering if it makes sense to drop pyright entirely. There were some good reasons to use it originally (see the notes from c073712), but as of #337 we are using mypy again anyway. Given that mypy is stricter, it seems a bit redundant to use pyright as well. I'll open a separate issue to get input on that question.

@schwehr
Copy link
Collaborator Author

schwehr commented Jun 1, 2021

./scripts/test 

 -- CHECKING TYPES WITH MYPY --

Success: no issues found in 77 source files

 -- CHECKING TYPES WITH PYRIGHT --

Checking node version...
Checking node_modules dir...
Checking pyright exists...
...installing pyright

changed 1 package, and audited 2 packages in 1s

found 0 vulnerabilities
done.
scripts/pyright: line 73: pyright: command not found

Where line 73 is:

pyright "$@"

Now I see the key problem is that the pyright script is not in the path. So it never gets a chance to try to install pyright.

@duckontheweb
Copy link
Contributor

Closed via #417

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

No branches or pull requests

2 participants