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: Typing hint for builder decorator #740

Merged
merged 2 commits into from
Sep 22, 2023

Conversation

gavindsouza
Copy link
Contributor

Maintain that the Callable type received is the same as the Callable returned.

Additional Context for the screenshots: tbl is a Table object, frappe.qb is a namespace that has type MySQLQuery | PostgreSQLQuery

Before

before-typing-change

After

after-typing-change

@AzisK
Copy link

AzisK commented Sep 7, 2023

Could you elaborate on this change a little bit more? I don't fully get it

@gavindsouza
Copy link
Contributor Author

@AzisK The previous type hints suggested that the builder function took a Callable input and returned a Callable but they need not be a Callable of the same type. This would confuse the type checkers while evaluating method chains, as you can see from the unhighlighted recurring where and delete chains in the "Before" section.

The change I'm proposing says that the Callable returned is that of the same type as the input received - which is of a query builder type that implements the accessed methods. Type checkers understand this and the consequence is that your IDE provides appropriate type hints as highlighted in the "After" section.

The example is usage from Frappe's codebase.

@AzisK
Copy link

AzisK commented Sep 8, 2023

Interesting. Okay, thanks for the explanation as well as for your work

Copy link

@AzisK AzisK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@AzisK
Copy link

AzisK commented Sep 8, 2023

We need to lint the code as well as fix the unit test runners

@gavindsouza
Copy link
Contributor Author

@AzisK We should add a pre-commit config that "black"en's the codebase automatically by the way.

--

As for the failing 3.6 build, it's high time we drop it given that it's unmaintained and there are 6 new major versions released meanwhile - #747

@AzisK
Copy link

AzisK commented Sep 8, 2023

I agree about the pre-commit hook. I just created an issue #748

I believe we can drop support for Python3.6, I am just not sure what to do with the version of the library. Do we have to bump the major version in this case?

@AzisK
Copy link

AzisK commented Sep 21, 2023

@gavindsouza could you update this PR with the newest CI code from master?

Maintain that the Callable type received is the same as the Callable returned
@coveralls
Copy link

Coverage Status

coverage: 98.423%. remained the same when pulling cc2d25b on gavindsouza:fix-typing-builders into a1b0c82 on kayak:master.

@gavindsouza
Copy link
Contributor Author

@gavindsouza could you update this PR with the newest CI code from master?

Done 😄

@AzisK
Copy link

AzisK commented Sep 22, 2023

Good job. I am merging this but new version and release will be coming later

@AzisK AzisK merged commit ea9cc5b into kayak:master Sep 22, 2023
5 checks passed
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