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

Fix mypy linting #1

Closed
wants to merge 2 commits into from
Closed

Fix mypy linting #1

wants to merge 2 commits into from

Conversation

tesmond
Copy link

@tesmond tesmond commented Apr 5, 2023

Change

  • adjusted type hints to pass mypy
  • remove unnecessary type hint ignores
  • black re-format

@tesmond tesmond closed this by deleting the head repository Apr 18, 2023
@farahats9
Copy link
Owner

@tesmond Sorry I didn't have the time to look at this, should I take a look or did you close it because it has something wrong?

@ArVar
Copy link

ArVar commented Jun 27, 2023

@farahats9 , it looks promising and could fix the problems, preventing the merge of your PR into sqlmodel. I would recommend to take a look at it.

@tesmond
Copy link
Author

tesmond commented Jun 27, 2023

@farahats9 sorry, I created the fork and made the changes, however, you can only open 1 fork per original repo. I thought instead I could take a direct fork of sqlmodel and get that working and make sure the tests passed etc. However, I did not get round to it.

You are welcome to use the changes, I do not think there was anything wrong, just I do not know if it would be enough for all the tests to pass in the original repo.

@ArVar
Copy link

ArVar commented Jun 28, 2023

@tesmond I had look into it. There are two clusters of problems. One is a mypy issue/bug with overloaded methods and their signatures . The other cluster is related to the way TypeVar handles definitions without bounds. This you can solve easily like here: 9266acb
There is also another issue regarding Forward References for recent sqlalchemy. This I couldn't further investigate.

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.

3 participants