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 mypy and fix exsisting typing issues #639

Closed
wants to merge 12 commits into from

Conversation

shroominic
Copy link

@shroominic shroominic commented Jun 3, 2024

Why mypy?
++ improved understanding for newbies
++ quickly seeing data flow of functions
++ less errors
++ easier refactoring
++ null safety
-- more verbose code
-- small learning curve

Optional:

  • add mypy config file
  • enable strict mypy + disallowing untyped defs for even more type safety

@paul-gauthier
Copy link
Collaborator

Thanks for all the effort here. I know that typing adds a lot of goodness. My main concern is that I haven't personally worked much in python code bases that are heavily typed. I'm a bit worried it will slow down development, and velocity is one of my highest priorities.

@shroominic
Copy link
Author

shroominic commented Jun 12, 2024

Yeah I think your are right in the beginning it will slow down development a bit but then i think it will actually be faster than before because you wont have certain types of bugs anymore and code written by language models will be more accurate because they have more information to work with especially the function signatures (important for RepoMap) are filled with more information.
Additionally mypy can be used for debugging and will improve code accuracy since its one more form of analysis similar to ruff/flake8.

@youknow04
Copy link
Contributor

I would like to add my experiences as someone who likes both typed python code and Aider.

Although type hints make the code a bit longer, I think they will accelerate development speed rather than slowing it down in a codebase the size of Aider.
Especially when pair programming with an LLM, I'm experiencing that type hints act as useful context to generate better code from LLM.
However, there is an issue that most LLMs (including GPT-4o) are not familiar with the built-in typing(Python 3.9+) syntax used in this PR.

Personally, I prefer built-in typing, so I don't use legacy typing syntax like "typing.Optional" and "typing.Dict", but LLMs have a strong tendency to use the legacy syntax.
In my personal project, I added "convention.md" for Adier, to add type hints using the built-in syntax and even provide few-shots, but I couldn't totally eliminate the tendency.

@shroominic
Copy link
Author

fully agree with @youknow04 and would love to see your convention.md if you could maybe gist that would be great!
Also to add to that I used 3.9+ typing and not 3.10+ so typing.Optional is still used but when we higher the requirements we can adjust to " | None" typing

@youknow04
Copy link
Contributor

@shroominic This is part of my current convention.md for Python.
I just found that I am now using one example, but once tried a long prompt with multiple examples including List, Tuple, and Dict. However, it still could not completely eliminate the tendency to use the legacy typing module.
This could be annoying because one should repeatedly convert legacy types from LLMs to built-in types.

- Python code:
    - Use Python 3.11
    - Annotate all types using the new built-in forms when possible (e.g., use "dict" instead of "Dict" from the typing module)
    - Ensure the code passes Pyright checks with the strict option enabled

@ricklamers
Copy link

This is amazing! This will definitely benefit the project, especially because new contributors will find it easier to reason about the implementation without having too much historical context (the inevitable coding by convention in untyped codebases). Awesome work @shroominic

@benjamin-kirkbride
Copy link

@paul-gauthier is this something that you are open to? What would you need to see / what would need to be done to convince you to merge this?

As a prospective contributor, having typing enforced makes me feel much more comfortable getting involved :)

@paul-gauthier
Copy link
Collaborator

I'm probably not going to take the plunge on trying to type hint the codebase at this time. And this PR is quite out of sync with the repo now. So I'm going to close it.

@akaihola akaihola mentioned this pull request Aug 29, 2024
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