Pyright Configuration for CI Pipeline

[C-Loftus]

Discussed with Pokey at Cursorless meetup last weekend about Pyright CI. Posting this here as a discussion to evaluate if Pyright is useful for Cursorless in its CI pipeline.

Downside for Pyright integration

• Since we can't install Talon in CI, we essentially have to ignore all Talon typing and imports unless we import typing stubs or something analogous

error: Import "talon" could not be resolved (reportMissingImports)

• Talon scripting with Python has development patterns that deviate from the norm so we have to ignore some errors similar to the following

error: Type of parameter "key" must be a supertype of its class "Actions" (reportGeneralTypeIssues)

• Not entirely sure why you can't get around the "type of parameter X must be a supertype" but since it throws an error you essentially have to disable all reportGeneralTypeIssues to get rid of it.
• submitted issue to https://github.com/microsoft/pyright/issues/7513

Current Config

[tool.pyright]
# Talon classes don't use self so ignore the associated errors
reportSelfClsParameterName = false
# Talon can't be installed in CI so ignore source errors
reportMissingModuleSource = false
reportMissingImports = false

May or may not want the following

# Ignore the type of parameter X must be a supertype of its class
# reportGeneralTypeIssues = false

[C-Loftus]

Feel free to comment on the Pyright issue if you feel I misrepresented anything. I will add a CI test if you think it is worthwhile, but wasn't sure if we can even run that in the branch before it is merged in a PR

[C-Loftus]

@pokey do you have opinions on this? https://github.com/microsoft/pyright/issues/7513#issuecomment-2004576594 I am frankly not sure. Adding @staticmethod doesn't seem to affect behavior but I never see anyone do in Talon so wasn't sure

[pokey]

@pokey do you have opinions on this? microsoft/pyright#7513 (comment) I am frankly not sure. Adding @staticmethod doesn't seem to affect behavior but I never see anyone do in Talon so wasn't sure

huh not sure why that never occurred to me; I've forwarded the question to Slack

[pokey]

I think this PR is missing the github action; can you not just copy the one from ai-tools verbatim?

[C-Loftus]

Added, not sure exactly where we want it in the Pipeline, but I put it at the end after the other tests. I don't have as sophisticated a pipeline of course so I had it as its own workflow but probably doesn't make sense here

[C-Loftus]

Seems like the general type feedback is going to be out of scope for pyright support. Mypy is an option that may support ignoring the first argument and not treating it as self, but I believe that requires converting the python folders into modules with __init__.py

[C-Loftus]

Ok so adding a new feature to Pyright to support this is out of scope.

However, https://github.com/microsoft/pyright/issues/7513#issuecomment-2005800660 made a good comment, namely that you can inherit from object

I feel like this is a bit of a hack and perhaps not worth it, but it is an option

# pyright: reportSelfClsParameterName=false

from typing import Any, TYPE_CHECKING

if TYPE_CHECKING:
    Base = Any
else:
    Base = object

class Actions(Base):
    def vscode_get_setting(key: str, default_value: Any = None):
  

[auscompgeek]

If mypy handles action classes the way we want, shouldn't we try running mypy in CI instead?

[C-Loftus]

If mypy handles action classes the way we want, shouldn't we try running mypy in CI instead?

I might be missing something but I am not finding it handles the action classes either. I think the best way is just adding #type:ignore comments inline (either Pyright or Mypy works for this) if we do even want to bother with all this. I think it makes contributing a bit more annoying to have to specify this so may not be worth it.

mypy output

PS C:\Users\cloftus\github\cursorless> mypy --namespace-packages --explicit-package-bases .\cursorless-talon-dev\
cursorless-talon-dev\src\default_vocabulary.py:1: error: Cannot find implementation or library stub for module named "talon"  [import-not-found]
cursorless-talon-dev\src\cursorless_dev.py:3: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
cursorless-talon-dev\src\cursorless_dev.py:10: error: Method must have at least one argument. Did you forget the "self" argument?  [misc]     
...

[dyamito]

Dropping this here to include reasons for picking one over the other: https://microsoft.github.io/pyright/#/mypy-comparison

[pokey]

Hmm it does seem like there would be a lot of compromises to get this one to work. Tbh one of the biggest issues from my perspective is the lack of Talon type stubs in CI. If I'm understanding correctly, we'd need to add flags to our pyright config to make CI happy without these stubs, but that would in effect weaken our type checking locally, where we do have the stubs. If that's the case, I'd be inclined to just close for now and revisit if the typing story in Talon changes in the future. @lunixbochs: long shot, but I'm assuming there's no chance of getting Talon stubs published as a pip package for use in CI? I'm guessing it would be too much overhead for you, but figured I'd check. Even if we got those, tho, we'd still have https://github.com/microsoft/pyright/issues/7513, so I'm not sure we'd be comfortable switching on these CI checks anyway

[C-Loftus]

I’m inclined to agree. If we can’t do anything with Talon in CI and we just have to ignore it, the only benefit here would be that it adds some baseline sanity checks to make sure nothing is egregiously wrong when committed.

On Mon, Apr 15, 2024 at 11:15 AM Pokey Rule @.***> wrote:

Hmm it does seem like there would be a lot of compromises to get this one to work. Tbh one of the biggest issues from my perspective is the lack of Talon type stubs in CI. If I'm understanding correctly, we'd need to add flags to our pyright config to make CI happy without these stubs, but that would in effect weaken our type checking locally, where we do have the stubs. If that's the case, I'd be inclined to just close for now and revisit if the typing story in Talon changes in the future. @lunixbochs https://github.com/lunixbochs: long shot, but I'm assuming there's no chance of getting Talon stubs published as a pip package for use in CI? I'm guessing it would be too much overhead for you, but figured I'd check. Even if we got those, tho, we'd still have microsoft/pyright#7513 https://github.com/microsoft/pyright/issues/7513, so I'm not sure we'd be comfortable switching on these CI checks anyway

— Reply to this email directly, view it on GitHub https://github.com/cursorless-dev/cursorless/pull/2267#issuecomment-2057105312, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQ2T6Z4LOD7GWJ4LAMFBQ4DY5PVGJAVCNFSM6AAAAABE4BEMTCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJXGEYDKMZRGI . You are receiving this because you authored the thread.Message ID: @.***>

[pokey]

Yes but I think it actually has downsides, because we'd have to add extra ignore flags to our pyright config that would weaken type-checking locally, where we do have the stubs. If it were just weakly useful in CI and neutral locally, I might go for it, but it seems like it's weakly useful in CI and detrimental locally, so I think that tips the balance in favour of closing

[fidgetingbits]

Maybe I'm missing something, but why not just install talon in CI? You could should be able to install it with nix package manager.

[pokey]

Maybe I'm missing something, but why not just install talon in CI? You could should be able to install it with nix package manager.

Talon's license doesn't allow us to install it in CI