Autogenerated FindBy #92

dpordomingo · 2017-02-13T09:31:17Z

solves #39

Functionality

Kallax will auto-generate the FindBy of each Model property if it satisfies one of the following;

A property will cause an auto-generated FindBy if:

its type is a "Valid" one:
- a) "Basic", one of the following: string, byte, bool, float(s), int(s) or uint(s) (see)
- b) "Special" (see)
  - one of the "kallaxKnownTypes" (SQL already knows how to handle it) (see)
  - a "SQLType" (implements sql.Scanner and driver.Valuer, see)
its type is a "collection" of "Valid" types

Type aliases will be valid if one of its underlying types is a "Valid" one.

Generated FindBy

The auto-generated FindBy for a given property named "PropertyName" will be as it follows:

model.ID -> FindByID(v ...kallax.ID)
the query will success if the model.ID is one of the passed kallax.ID
"Basic" and "Special" properties
- string and boolean properties -> FindByPopertyName(v validType),
  the query will success if the model property is equal to the passed kallax.ID
- comparable properties -> FindByPopertyName(cond kallax.ScalarCond, v validType),
  the query will success if the model property satisfies the passed kallax.ScalarCond
"Collection" properties -> FindByPopertyName(v ...singleValidType)
the query will success if the model collection property contains one of the passed values

dpordomingo · 2017-02-13T20:36:35Z

affected by #93

erizocosmico

sorry for the amount of requested changes, most of them are small stuff, naming of things and requesting tests

erizocosmico · 2017-02-14T09:11:29Z

generator/findby.go

+)
+
+const (
+	tplFindByInclude = "\n\n// FindBy adds a new filter to the query that will require that\n" +


we should use the ` for multiline strings instead of concatenated strings, it would be easier to read imho

much better

erizocosmico · 2017-02-14T09:14:48Z

generator/types.go

@@ -106,7 +106,7 @@ var reservedKeywords = map[string]struct{}{

 // special types that are not analyzed because SQL already knows
 // how to handle them
-var specialTypes = map[string]string{
+var kallaxKnownTypes = map[string]string{


why the rename of this?

erizocosmico · 2017-02-14T09:16:10Z

generator/findby.go

+	return s
+}
+
+func isEqualizable(f *Field) bool {


some docs for these 3 functions would be nice for context

erizocosmico · 2017-02-14T09:20:15Z

generator/findby.go

+// string, byte, bool, float(s), int(s) or uint(s)
+func isBasicType(pkg *types.Package, typ types.Type) bool {
+	s := typ.String()
+	return s == "string" || s == "bool" || s == "byte" || s == "float64" || s == "float32" ||


what do you think about converting this to a switch, which would be cleaner?

switch s { case "string", "bool", "byte", "float64", ...: return true } return false

tricky... but ok :)

erizocosmico · 2017-02-14T09:20:48Z

generator/findby.go

+	return cleanName(typ, true)
+}
+
+func cleanName(typ types.Type, requireShortName bool) string {


this is already implemented https://github.com/src-d/go-kallax/blob/master/generator/types.go#L697

this needs tests

erizocosmico · 2017-02-14T09:45:45Z

generator/findby.go

+// validType returns the short name of the type that can be used to search for
+// the passed Field, in an autogenerated 'FindBy';
+// if the Field is not valid for an autogenerated 'FindBy' it will return an error
+func validType(f *Field) (string, error) {


what do you think about naming this findableTypeName?

this should have tests

erizocosmico · 2017-02-14T09:49:20Z

generator/findby.go

+
+// GenFindBy generates FindByPropertyName for all model properties that are
+// valid types or collection of valid types.
+func (td *TemplateData) GenFindBy(model *Model) string {


this should have tests

good idea ;)

erizocosmico · 2017-02-14T09:50:31Z

generator/findby.go

+	return ok || isSQLType(pkg, typ)
+}
+
+func kallaxKnownTypeName(typ types.Type) (string, bool) {


if the name of kallaxKnownTypes changes, this should change too

did't changed :)

erizocosmico · 2017-02-14T09:51:00Z

generator/findby.go

+
+// isSpecialType returns true if the passed type is one of the kallaxKnownTypes
+// or a types.SQLType
+func isSpecialType(pkg *types.Package, typ types.Type) bool {


if kallaxKnownTypes changes, this should change too

did't changed :)

erizocosmico · 2017-02-14T09:58:11Z

tests/query_test.go

+
+func (s *QuerySuite) TestGeneration() {
+	q := NewQueryFixtureQuery()
+	s.hasFindByMethod(q, "ID")


how about a trable-driven tests?

var cases = []struct{ name string generated bool }{ {"ID", true}, // ... } for _, c := range cases { if c.generated { s.hasFindByMethod(q, c.name) } else { s.hasNotFindByMethod(q, c.name) } }

that way it's easier to add more cases and there is less repeated code

erizocosmico

when tests are implemented lgtm

erizocosmico · 2017-02-14T14:25:50Z

generator/findby.go

 	if special, ok := specialTypeShortName(typ); ok {
 		return special
 	}

 	return cleanName(typ, true)
 }

+//TODO: review existent implementation generator.types::typeString


space after //

erizocosmico · 2017-02-14T14:27:12Z

Btw, a lot of tests are failing

dpordomingo · 2017-02-14T14:34:04Z

@erizocosmico many thanks for your recommendations!!!
I applied most of your suggested changes but:

moving findby.go into template.go in order to make it easier the sequential review of the PR,
adding tests, that will be done from now on, since we agree in the api and global behavior,
using generator.typeString instead of generator.cleanName, after the tests (of the previous step) has been done, to ensure that this changes breaks nothing.

dpordomingo · 2017-02-14T17:25:11Z

affected by #96

codecov-io · 2017-02-15T08:57:19Z

Codecov Report

Merging #92 into master will increase coverage by 0.41%.
The diff coverage is 92.39%.

@@            Coverage Diff             @@
##           master      #92      +/-   ##
==========================================
+ Coverage    80.1%   80.51%   +0.41%     
==========================================
  Files          15       15              
  Lines        2568     2659      +91     
==========================================
+ Hits         2057     2141      +84     
- Misses        348      354       +6     
- Partials      163      164       +1

Impacted Files	Coverage Δ
generator/types.go	`86.56% <ø> (ø)`	✅
operators.go	`92.85% <ø> (ø)`	✅
generator/processor.go	`88.93% <100%> (+0.08%)`	✅
generator/template.go	`90.56% <92.13%> (+0.56%)`	✅

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 309f22e...14519e0. Read the comment docs.

erizocosmico · 2017-02-16T15:17:13Z

Is this ready for review again? @dpordomingo

dpordomingo · 2017-02-17T08:57:31Z

I forgot pushing it;
58877ad Use generator.types::typeString as suggested by @erizocosmico
a11cfb2 Add test for autogenerated FindBy

Pending, and will be done after your validation, in order to make it easier the sequential review of the PR :

moving findby.go into template.go in order to make it easier the sequential review of the PR,
squash and rebase after your validation

erizocosmico · 2017-02-17T14:31:42Z

@dpordomingo so is it ready for review, right?

dpordomingo · 2017-02-17T14:38:49Z

Yes, it is :)

erizocosmico · 2017-02-17T15:31:51Z

tests/query_test.go

+	})
+
+	s.NotPanics(func() {
+		//s.True(store.MustFindOne(NewQueryFixtureQuery().FindByBoolean(false)).Eq(queryFixtures[1]))


nope,
Those tests are already broken tests, affected by #96

I updated it in my rebased branch, and it looks this way:

Oh, ok, then lgtm

Serabe · 2017-02-20T09:25:17Z

@dpordomingo tests are failing. Are they supposed to fail yet?

dpordomingo · 2017-02-20T10:50:37Z

@erizocosmico @Serabe Tests now passes, it was needed to use the go-parse-utils importer 58a187c

Serabe

Disregard the test thing.

Serabe · 2017-02-20T11:12:49Z

generator/template.go

+		case isEqualizable(f):
+			writeTplCode(buf, parent, f, tplFindByEquality)
+		case isSortable(f):
+			writeTplCode(buf, parent, f, tplFindByCondition)


This path is not tested.

Fix import problem in template_test

dpordomingo force-pushed the findBy branch 2 times, most recently from f0d1c31 to 13630ff Compare February 13, 2017 18:56

dpordomingo requested review from Serabe, erizocosmico and mcuadros February 13, 2017 19:03

dpordomingo force-pushed the findBy branch from 13630ff to e324dd1 Compare February 13, 2017 19:07

erizocosmico suggested changes Feb 14, 2017

View reviewed changes

dpordomingo force-pushed the findBy branch 2 times, most recently from 8eb97f9 to 8396838 Compare February 14, 2017 15:36

dpordomingo force-pushed the findBy branch from 1cf1f56 to b7e7c55 Compare February 15, 2017 08:54

Serabe approved these changes Feb 16, 2017

View reviewed changes

dpordomingo force-pushed the findBy branch from 2207abf to a11cfb2 Compare February 17, 2017 08:53

erizocosmico suggested changes Feb 17, 2017

View reviewed changes

erizocosmico approved these changes Feb 17, 2017

View reviewed changes

dpordomingo force-pushed the findBy branch 2 times, most recently from 49dc8e0 to 90df9e8 Compare February 20, 2017 05:18

erizocosmico added the feature label Feb 20, 2017

dpordomingo force-pushed the findBy branch from 596adf4 to 58a187c Compare February 20, 2017 10:42

dpordomingo force-pushed the findBy branch from 58a187c to 14519e0 Compare February 20, 2017 11:13

Serabe approved these changes Feb 20, 2017

View reviewed changes

dpordomingo added 2 commits February 20, 2017 12:17

AutoGenerate FindBy query methods for Basic and SQLTypes properties

92b7dfa

Fix import problem in template_test

Skip tests affected by issue#96

a2e945e

dpordomingo force-pushed the findBy branch from 14519e0 to a2e945e Compare February 20, 2017 11:17

dpordomingo merged commit e32044f into src-d:master Feb 20, 2017

dpordomingo deleted the findBy branch February 20, 2017 12:04

Autogenerated FindBy #92

Autogenerated FindBy #92

Conversation

dpordomingo commented Feb 13, 2017 • edited

Functionality

Generated FindBy

dpordomingo commented Feb 13, 2017

erizocosmico left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erizocosmico left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erizocosmico commented Feb 14, 2017

dpordomingo commented Feb 14, 2017

dpordomingo commented Feb 14, 2017

codecov-io commented Feb 15, 2017 • edited

Codecov Report

erizocosmico commented Feb 16, 2017

dpordomingo commented Feb 17, 2017

erizocosmico commented Feb 17, 2017

dpordomingo commented Feb 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Serabe commented Feb 20, 2017

dpordomingo commented Feb 20, 2017

Serabe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpordomingo commented Feb 13, 2017 •

edited

codecov-io commented Feb 15, 2017 •

edited