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.

Sign up for GitHub

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

Jump to bottom

Add method Selection::havingOr #243

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Add method Selection::havingOr #243

wants to merge 1 commit into from

Conversation

dakujem
Copy link

@dakujem dakujem commented Nov 26, 2019

Adds Selection::havingOr method analogously to the Selection::whereOr, refactoring the common logic into a separate protected method (Selection::paramsOr).

closes #240

@dakujem
Copy link
Author

dakujem commented Nov 26, 2019

Hello, David. Does this look good to you? I will eventually solve the currently present issues, I hope. I'm in a hurry now though.

Sidenote: I spent unnecessary time to find out how to run the tests so that most of them are not skipped... May I suggest adding a brief info for other devs how to run the tests?

@dakujem
Copy link
Author

dakujem commented Nov 26, 2019

Another sidenote: I was wondering why are the two methods (where and having) so inconsistent. Why does having only accept a string, while where will happily accept an array of string as well? All the while multiple calls to having will overwrite the HAVING condition, while multiple calls to where will add more WHERE conditions. This I find suboptimal, as both HAVING and WHERE are similar in the way the resulting SQL is built.

@dg
Copy link
Member

dg commented Nov 26, 2019

Does this look good to you?

Thanks, it looks great!

I spent unnecessary time to find out how to run the tests so that most of them are not skipped... May I suggest adding a brief info for other devs how to run the tests?

That would be great, I don't realize these things. Maybe add it to a the file contributing.md?

I was wondering why are the two methods (where and having) so inconsistent.

Unfortunately, I don't know, I'm not an author. It could be unified, but I don't know how big a BC break would be.

@dg dg force-pushed the master branch 6 times, most recently from f9fc19d to dd75e74 Compare December 13, 2019 18:06
@dg dg force-pushed the master branch 2 times, most recently from cce6a9c to 7e705c5 Compare December 27, 2019 03:53
@dg dg force-pushed the master branch 2 times, most recently from 9acf825 to a268c60 Compare March 27, 2020 11:52
@dg dg force-pushed the master branch 2 times, most recently from a56e7fe to d9b5f29 Compare June 9, 2020 14:19
@dg dg force-pushed the master branch 2 times, most recently from f016c33 to 6596c77 Compare July 10, 2020 16:30
@dg dg force-pushed the master branch 2 times, most recently from a9e5763 to c3cc31a Compare October 6, 2020 17:24
@dg dg force-pushed the master branch 2 times, most recently from e91b234 to 953502b Compare October 15, 2020 22:53
@dg dg force-pushed the master branch 5 times, most recently from f8de9c7 to 52d72e1 Compare October 29, 2020 22:59
@dg dg force-pushed the master branch 29 times, most recently from a329d1b to 9f82d9d Compare September 5, 2024 02:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Missing havingOr ?
2 participants
@dakujem @dg