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

Pyright Configuration for CI Pipeline #2267

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

C-Loftus
Copy link
Contributor

@C-Loftus C-Loftus commented Mar 18, 2024

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)

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 C-Loftus marked this pull request as draft March 18, 2024 17:35
@C-Loftus
Copy link
Contributor Author

C-Loftus commented Mar 18, 2024

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 C-Loftus changed the title Pyright Config Draft Pyright Configuration for CI Pipeline Mar 18, 2024
@C-Loftus
Copy link
Contributor Author

@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
Copy link
Member

pokey commented Mar 18, 2024

@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
Copy link
Member

pokey commented Mar 18, 2024

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

@C-Loftus
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

C-Loftus commented Mar 19, 2024

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):
  

@auscompgeek
Copy link
Member

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

@C-Loftus
Copy link
Contributor Author

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
Copy link

dyamito commented Mar 20, 2024

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

@pokey
Copy link
Member

pokey commented Apr 15, 2024

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

@C-Loftus
Copy link
Contributor Author

C-Loftus commented Apr 15, 2024 via email

@pokey
Copy link
Member

pokey commented Apr 22, 2024

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

github-merge-queue bot pushed a commit that referenced this pull request Apr 28, 2024
- Depends on #2306
- Supercedes #2267

This does add a lot of noise, and it can't be enforced in CI because
Talon doesn't publish type stubs (that I know of), so I wonder whether
it's worth it 🤔

## Checklist

- [x] I have run Talon spoken form tests
- [-] I have added
[tests](https://www.cursorless.org/docs/contributing/test-case-recorder/)
- [-] I have updated the
[docs](https://github.com/cursorless-dev/cursorless/tree/main/docs) and
[cheatsheet](https://github.com/cursorless-dev/cursorless/tree/main/cursorless-talon/src/cheatsheet)
- [x] I have not broken the cheatsheet
cursorless-bot pushed a commit to cursorless-dev/cursorless-talon that referenced this pull request Apr 28, 2024
- Depends on cursorless-dev/cursorless#2306
- Supercedes cursorless-dev/cursorless#2267

This does add a lot of noise, and it can't be enforced in CI because
Talon doesn't publish type stubs (that I know of), so I wonder whether
it's worth it 🤔

## Checklist

- [x] I have run Talon spoken form tests
- [-] I have added
[tests](https://www.cursorless.org/docs/contributing/test-case-recorder/)
- [-] I have updated the
[docs](https://github.com/cursorless-dev/cursorless/tree/main/docs) and
[cheatsheet](https://github.com/cursorless-dev/cursorless/tree/main/cursorless-talon/src/cheatsheet)
- [x] I have not broken the cheatsheet
cursorless-bot pushed a commit that referenced this pull request Apr 28, 2024
- Depends on #2306
- Supercedes #2267

This does add a lot of noise, and it can't be enforced in CI because
Talon doesn't publish type stubs (that I know of), so I wonder whether
it's worth it 🤔

## Checklist

- [x] I have run Talon spoken form tests
- [-] I have added
[tests](https://www.cursorless.org/docs/contributing/test-case-recorder/)
- [-] I have updated the
[docs](https://github.com/cursorless-dev/cursorless/tree/main/docs) and
[cheatsheet](https://github.com/cursorless-dev/cursorless/tree/main/cursorless-talon/src/cheatsheet)
- [x] I have not broken the cheatsheet
@fidgetingbits
Copy link
Collaborator

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
Copy link
Member

pokey commented Aug 1, 2024

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

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

Successfully merging this pull request may close these issues.

5 participants