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: Improve return type hints and return docblocks for query classes #470

Merged
merged 1 commit into from
Apr 11, 2024

Conversation

olliescase
Copy link
Contributor

First of, thank you for this package! Its a great library that has had a great and positive impact on our work.

While using this package I found that the type-hinting for some of the methods wasn't optimal. This meant that using functions such as fetchOrderDesc meant you could no longer receive type hints for the WhereQuery methods.

I have implemented two changes in this PR:

Firstly, I have re-typed all function return type hints to static. This is more truthful than saying it returns itself -- if you were to extend these classes (such as the WhereQuery does the Query) then the reality is that it will return the subclass when these methods are called; hence static as its context aware.

Secondly, I have changed all the return docblocks to @return $this where they were previously set otherwise. Most of the functions followed this pattern anyway, but there were a couple that seemed to be forgotten about.

Functionally, nothing has changed - but it allows editors to decipher and understand what is happening more easily.

If you'd prefer to not change the return type hints and only do this through the docblocks, I am happy to revert the change to the return type hints.

Many thanks,
Ollie Scase
~ techlove

@Webklex
Copy link
Owner

Webklex commented Apr 11, 2024

Hi @olliescase ,
many thanks for your suggested improvements and your pull request!

Best regards & happy coding,

@Webklex Webklex merged commit e5e8eb0 into Webklex:master Apr 11, 2024
3 checks passed
@stevebauman stevebauman mentioned this pull request Jan 30, 2025
18 tasks
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