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

added type hints #1171

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jayashankarjayan
Copy link

Added type hints to variables, function arguments:

Relevant issue #297

Pre-Submission Checklist (optional but appreciated):

Code formatting applied using black and isort as per guidelines.

  • I have included relevant documentation updates (stored in /docs)
  • I have read docs/CONTRIBUTING.md
  • I have read docs/ROADMAP.md

OS Tests (optional but appreciated):

  • Tested on Windows
  • Tested on MacOS
  • Tested on Linux

@imapersonman
Copy link
Contributor

These will be really nice to have! It looks like there are some inconsistencies in the type hinting, though. For example, messages has type str | None as an OpenInterpreter parameter in core, but is being stored as a list[dict[str, Any]] in the OpenInterpreter constructor's first line. You can turn on type checking using VSCode + Pylance's Python › Analysis: Type Checking Mode option, or use pyright on the command line.

I think it would be really cool to have this merged once the typing is consistent.

@jayashankarjayan
Copy link
Author

jayashankarjayan commented Apr 8, 2024

These will be really nice to have! It looks like there are some inconsistencies in the type hinting, though. For example, messages has type str | None as an OpenInterpreter parameter in core, but is being stored as a list[dict[str, Any]] in the OpenInterpreter constructor's first line. You can turn on type checking using VSCode + Pylance's Python › Analysis: Type Checking Mode option, or use pyright on the command line.

I think it would be really cool to have this merged once the typing is consistent.

Thanks @imapersonman!. The reason why I have kept messages type as str | None instead of just str is that the constructor of OpenInterpreter class actually has messages as an optional keyword argument, i.e. it can be None.

Edit: On a second look at the code, I think I get your point. I have updated the type hint for messages keyword argument and added some more type hints, generally speaking

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.

2 participants