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 pyright as an optional tool #612

Merged
merged 1 commit into from
May 7, 2023

Conversation

lukaspiatkowski
Copy link
Contributor

Description

This PR adds the PyrightTool and registers pyright as an optional tool with prospector. One caveat is that this relies on the unofficial python package that wraps the official npm package, although the official documentation recommends using the community-maintained wrapper for some use cases.

Motivation and Context

The motivation is the same as with mypy which is just another type checker.

Note that Prospector's profiles are not being used in this PR (same as with mypy) since pyright doesn't enable much customization through command line arguments. Nevertheless, having prospector unifying pyright output is very beneficial - I've recently added a PyCharm plugin for Prospector which could be used to get pyright results through prospector in PyCharm (pyright doesn't have its own plugin in PyCharm) and I imagine other IDEs/systems might be in the same position.

How Has This Been Tested?

Basic tests were added (based on MypyTool tests). I've also run prospector --with-tool pyright -t pyright with various options set in .prospector.yaml.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • My change requires a change to the dependencies
  • I have updated the dependencies accordingly
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Copy link
Collaborator

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Looks great, imo, thank you for the work, clearly care went into it. I'm not using pyright myself and I don't know if the mirror used is official enough that we can trust it. So I'm not able to take a decision on this.

@Pierre-Sassoulas Pierre-Sassoulas added this to the 1.10.0 milestone May 6, 2023
@lukaspiatkowski
Copy link
Contributor Author

Thanks for quick review! I can only point out that the community-maintained python wrapper has been tracking the official npm package for 2 years now. I think that given that it is officially recommended by the pyright maintainers to use it for pre-commit hooks it is likely that either it will prevail or they will pick it up for further maintenance.

@carlio
Copy link
Member

carlio commented May 7, 2023

I will also say thank you @lukaspiatkowski .

I have one question, which I couldn't figure out by reading the code of the python wrapper for pyright. Will it have a dependency on node even if this is not used?

What I mean is: does installing and importing it, but not actually running it, end up in it trying to find a node binary?

I just want to avoid people who are currently using [with_everything] having their builds break due to a tool that they don't use causing prospector to crash searching for node.

I don't think it does from what I can tell, but I'm sure you can answer it with more certainty than I can!

@lukaspiatkowski
Copy link
Contributor Author

lukaspiatkowski commented May 7, 2023

@carlio I believe that it lazily calls node to install pyright only when called here: https://github.com/RobertCraigie/pyright-python/blob/main/pyright/cli.py#L25 and that npm has to be installed on your own as the nodeenv dependency of that wrapper is just for initializing node virtual environment, it doesn't contain node itself.

So to answer you directly: I don't think it searches for node binary unless you run it.

@carlio
Copy link
Member

carlio commented May 7, 2023

Ok, sounds good. I just want to avoid any bug reports wondering why their python CI pipeline needs node :-)

I'm happy to merge in that case.

@carlio carlio merged commit c798436 into prospector-dev:master May 7, 2023
@carlio carlio mentioned this pull request May 7, 2023
11 tasks
@carlio
Copy link
Member

carlio commented May 7, 2023

As mentioned in #611, readthedocs isn't working due to a urllib3 release a couple of days ago.

I'm a bit tired now to fight this so I'll pick it up tomorrow.

@carlio
Copy link
Member

carlio commented May 12, 2023

Docs are fixed now, so Prospector 1.10.0 is on PyPI now with this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants