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

feat(select): add simple where utility functions #15

Merged
merged 1 commit into from
Aug 3, 2021

Conversation

lundibundi
Copy link
Member

  • whereEq
  • whereMore
  • whereMoreEq
  • whereLess
  • whereLessEq

* whereEq
* whereMore
* whereMoreEq
* whereLess
* whereLessEq
@lundibundi lundibundi self-assigned this Aug 8, 2020
Copy link
Member

@tshemsedinov tshemsedinov left a comment

Choose a reason for hiding this comment

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

This methods is ok, but later we need refactor all methods to declarative where syntax. It's better for storing and passing multiple where conditions in json format or as javascript object, less code, less methods, more declarative structures.

@belochub
Copy link
Member

In my opinion, these methods are useless since each of them can be replaced with builder.where() with a relevant comparison operator as a second argument. These methods introduce Polish (prefix) notation for these comparison operators which is, in my opinion, less readable, due to it being significantly different from the resulting SQL, thus I would always prefer using builder.where() instead of these methods.

@lundibundi
Copy link
Member Author

In my opinion, these methods are useless since each of them can be replaced with builder.where()

Well, the same goes for every method in this package as it can be replaced with direct SQL anyway. This PR just provides more convenience methods to build a query just like the ones we have.

These methods introduce Polish (prefix) notation for these comparison operators which is, in my opinion, less readable, due to it being significantly different from the resulting SQL,

Sure but other people may not think so and I see no harm in having a few more one-line methods that just delegate all of their functionality anyway and therefore won't introduce almost any maintenance burden.
It is not "significantly" different either from SQL or from the general .where method (.where('key', '>', 100) vs .whereMore('key', 100) vs "key" > 100). Moreover methods such as .whereNotNull are more different than this.

@tshemsedinov
Copy link
Member

Please consider multiple alternative syntaxes here: #14
@lundibundi @belochub

@lundibundi lundibundi merged commit b17327c into master Aug 3, 2021
@lundibundi lundibundi deleted the add-utility-methods branch August 3, 2021 16:40
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.

None yet

3 participants