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

Query Builder options #28

Merged
merged 5 commits into from
Jul 27, 2019
Merged

Query Builder options #28

merged 5 commits into from
Jul 27, 2019

Conversation

bashleigh
Copy link
Collaborator

Instead of only being limited to repositories, you an pass in a query builder to return the pagination object

#24

Copy link
Member

@shekohex shekohex left a comment

Choose a reason for hiding this comment

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

wouldn't it be better to expose the two methods instead of one method for both?

@bashleigh
Copy link
Collaborator Author

Hmmmm I dunno, I like the one method approach. I did export 2 declares though so it's like one method for 2 types? https://github.com/nestjsx/nestjs-typeorm-paginate/pull/28/files#diff-eb13222256edaba485eae8aeb826bc78R10 typeorm does it a lot so wanted to give it a go :p what do you think? @shekohex

@shekohex
Copy link
Member

I just don't like the monomorphism and polymorphism in general and i always try to avoid them when possible.

@michaelyali
Copy link
Member

SOLID 😉 There are several pros when your methods follow a single responsibility pattern. My favorite one is testing. But, of course, there are situations when you really can't (read don't want to) follow it, but I'm always trying to avoid those.

@johannesschobel
Copy link
Contributor

i would also have used 2 different methods.. but i guess this is a highly opinionated topic :D

@bashleigh
Copy link
Collaborator Author

bashleigh commented Jul 26, 2019

ok so 2 separate method. I liked the double export though 😂 never mind.

Copy link
Member

@fwoelffel fwoelffel 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 to me. I have no issue with the polymorphism since the code has been kept trivial and easily testable.

@adrien2p
Copy link
Member

I prefer when it respect the single responsibility principle, but that’s my point of view ^^

@nartc
Copy link

nartc commented Jul 26, 2019

I do prefer polymorphism since it makes the API more concise.

Copy link
Member

@shekohex shekohex left a comment

Choose a reason for hiding this comment

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

Okay i have no problem at all 😊

@bashleigh bashleigh merged commit 9ec9c46 into master Jul 27, 2019
@delete-merged-branch delete-merged-branch bot deleted the feature/querybuilder branch July 27, 2019 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants