Skip to content

Conversation

jorbs
Copy link

@jorbs jorbs commented Apr 25, 2017

No description provided.

@jorbs jorbs mentioned this pull request Apr 25, 2017
4 tasks
@codecov
Copy link

codecov bot commented Apr 25, 2017

Codecov Report

Merging #137 into master will decrease coverage by 0.14%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #137      +/-   ##
==========================================
- Coverage   80.37%   80.22%   -0.15%     
==========================================
  Files          15       15              
  Lines        2787     2792       +5     
==========================================
  Hits         2240     2240              
- Misses        375      380       +5     
  Partials      172      172
Impacted Files Coverage Δ
operators.go 89.92% <0%> (-3.63%) ⬇️

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 26fcb7f...38fdce5. Read the comment docs.

Copy link
Contributor

@erizocosmico erizocosmico left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution!

I'd like to ask you though for a few small changes just to keep this operator consistent with the existing ones.

s.Equal("bar", store.MustFindOne(queryErr).Foo)
})

foobar := newQueryFixture("foo bar")
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, move the operators tests to this test for consistency https://github.com/src-d/go-kallax/blob/master/operators_test.go#L34

Copy link
Author

Choose a reason for hiding this comment

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

Initially, I tried to run operators_test.go inside of my forked repo folder, but I always get can't load package: package github.com/jorbs/go-kallax: code in directory ~/go/src/github.com/jorbs/go-kallax expects import "gopkg.in/src-d/go-kallax.v1". I'm new on Go lang, but I think the packages are fixed to the original repo (github.com/src-d/go-kallax).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, they are fixed to gopkg.in/src-d/go-kallax.v1. What you can do is clone the repo at $GOPATH/src/gopkg.in/src-d/go-kallax.v1 and add your own remote there. Unfortunately, until real package management is a thing in Go, we have to stick with such methodologies.

Copy link
Author

Choose a reason for hiding this comment

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

I got it. Thanks.

values []interface{}
}

like struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

there's no need for a new struct, actually, there is colOp for that https://github.com/src-d/go-kallax/blob/master/operators.go#L286

// See https://www.postgresql.org/docs/9.6/static/functions-matching.html.
func Like(col SchemaField, value string) Condition {
return func(schema Schema) squirrel.Sqlizer {
return like{col.QualifiedName(schema), value}
Copy link
Contributor

Choose a reason for hiding this comment

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

@erizocosmico
Copy link
Contributor

Thanks! LGTM.

Can you remove the commit 3d89d2b, both changes show up right now

@erizocosmico
Copy link
Contributor

Don't worry, I already cherrypicked the commit and pushed it. Thanks!

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.

2 participants