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

[11.x] Improve type-hints in query builder #54299

Closed

Conversation

shaedrich
Copy link
Contributor

String type-hints are often times quite broad, even though, expected values might be a finite list. This PR attempts to better reflect the list of allowed values for several Builder methods.

@shaedrich shaedrich changed the title Improve string type-hints in query builder [11.x] Improve string type-hints in query builder Jan 22, 2025
@shaedrich shaedrich changed the title [11.x] Improve string type-hints in query builder [11.x] Improve type-hints in query builder Jan 22, 2025
@taylorotwell
Copy link
Member

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If applicable, please consider releasing your code as a package so that the community can still take advantage of your contributions!

@shaedrich
Copy link
Contributor Author

type-hints as a package—you are unintentionally funny and you don't even know that 😂

@dennisprudlo
Copy link
Contributor

type-hints as a package—you are unintentionally funny and you don't even know that 😂

That response is a general rejection template, which is why it includes the recommendation to create a package.

The truly unintentionally funny thing is that you already know this, since you never let a rejected PR go unnoticed and always make a fuss about it, yet you still pretend the response is meant to be taken literally, word for word.

If you think Taylor should respond to every closed PR with a unique message, that's fine. But for some reason the only GitHub notifications I get are you complaining about it – I think all of the people hanging around here gets the point by now.

@shaedrich
Copy link
Contributor Author

That response is a general rejection template, which is why it includes the recommendation to create a package.

I'm fully aware of that. Taylor doesn't bother to write one single line for any PR, nor does he care if the template fits the PR because he doesn't bother to read anything, as he has proven time and again.

He doesn't have to respond with a wall of text, but I die on that hill that it can be expected of a maintainer to communicate clearly with rejection reasons that make sense. But sure, go on shooting the messenger instead of pointing out Taylor's behavior 🤷🏻

@dennisprudlo
Copy link
Contributor

I simply don't see the point of responding with "I don't think we need that" in different ways just so one can say "the maintainer gives unique responses"

I also don't see what behavior there is to point out. If it's just his gut feeling that the contribution is not necessary then that's just it. He has every right to say "I don't think we need that" without presenting a report on why that is.

@shaedrich
Copy link
Contributor Author

shaedrich commented Jan 22, 2025

I simply don't see the point of responding with "I don't think we need that" in different ways just so one can say "the maintainer gives unique responses"

That's entirely not the point. This can be as easy as a label "rejected: hard to maintain" or the like. Even "I don't think we need that" is a little nondescript.

Also, giving vague rejection reasons leads to other people speculating and telling people, targeting another branch lead to Taylor "definitely merging" that, which he then—surprise, surprise!—rejects again. And even Taylor himself sometimes sends contradictory messages.

I also don't see what behavior there is to point out.

I'm not going to repeat myself more on that. You can read my other reactions you got all these notifications from if that interests you.

He has every right

He has every right to reject. Yet he has the responsibility for the rejection to make sense. His response via #54160 (comment) is just laughable and clearly shows that he reads nothing and goes with his gut feeling even though if it misleads him.

Another instance is #53661 vs #53379, which are almost verbatim. And don't tell me, the need for it changed within only 22(!) days. This is not a mistake but a logical consequence of his inattentiveness. And I'd say, it goes without saying that rejecting a PR because the reviewer didn't like one single character in the whole code is quite inefficient compared to just asking to change it real quick or even send a suggestion directly.

without presenting a report on why that is.

Yes and no. This is not a hobby project. Countless people are relying on it.

@dennisprudlo
Copy link
Contributor

Agree to disagree. Would still enjoy getting less of these notifications.

@shaedrich
Copy link
Contributor Author

Would still enjoy getting less of these notifications.

I honestly would be happy if could somehow exclude me from your notifications, but I assume, such level of customization isn't offered by GitHub 🤔

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