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

"Exists" method for store #236

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

Conversation

josephbuchma
Copy link

@josephbuchma josephbuchma commented Sep 21, 2017

Example usage:

us := NewUserStore(db)
exists, err := us.Exists(NewUserQuery().Where(kallax.Eq(Schema.User.Email, form.Email)))

@codecov
Copy link

codecov bot commented Sep 21, 2017

Codecov Report

Merging #236 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #236   +/-   ##
=====================================
  Coverage      79%    79%           
=====================================
  Files          19     19           
  Lines        3701   3701           
=====================================
  Hits         2924   2924           
  Misses        567    567           
  Partials      210    210

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8048762...4278c1c. Read the comment docs.

@roobre
Copy link
Contributor

roobre commented Sep 28, 2017

I'd say this functionality is already covered with Count(), which returns zero if no records are found. It looks a bit prettier tho, you have a point there.

@josephbuchma
Copy link
Author

josephbuchma commented Sep 28, 2017

I haven't looked at the code, so maybe I'm missing the point, but sql's count is not efficient way of performing "exists" check.

@roobre
Copy link
Contributor

roobre commented Sep 28, 2017

AFAIK, modern postgres versions use indices to perform simple counts. This should be quite fast (O(n), but in memory).

If I read your code correctly, you perform a query and call rs.Next(), which actually copies values to memory. Only the PK is copied tho (which I missed when I looked at it before), so your method is probably faster.

This seems good to me.

@roobre
Copy link
Contributor

roobre commented Nov 16, 2017

Now that I maintain this project actively, I'm interested on getting this merged. Please add some documentation and usage examples to README.md, as well as some tests to the suite. Then, unless anyone has anything to object, I'll merge it.

@josephbuchma
Copy link
Author

Hey @roobre , unfortunately for kallax I switched to sqlboiler. For now I don't have a time for finishing this, maybe later if it's not urgent. Otherwise example usage is provided in my first comment above, and testing this should be straight-forward, so you can get it merged.

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

2 participants