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

@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

@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

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

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

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

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

However, microsoft/pyright#7513 (comment) 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):
 

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

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] 
...

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

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 microsoft/pyright#7513, so I'm not sure we'd be comfortable switching on these CI checks anyway

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