Use constants for storage table names in repo tests#74
Conversation
| Metadata: storage.GetValueMetadata(cat.IlkLump, map[storage.Key]string{constants.Ilk: test_helpers.FakeIlk.Hex}, storage.Uint256), | ||
| PropertyName: "Lump", | ||
| PropertyValue: strconv.Itoa(rand.Int()), | ||
| TableName: "maker.cat_ilk_lump", |
There was a problem hiding this comment.
I was thinking of making a similar change in some of the other places we're hardcoding table names, but I'm not sure if that would reduce the readability of queries... ie. line 204
err = db.Get(&result, `SELECT diff_id, header_id, ilk_id AS key, lump AS value FROM maker.cat_ilk_lump`)
could become
err = db.Get(&result, `SELECT diff_id, header_id, ilk_id AS key, lump AS value FROM $1`,
shared.GetFullTableName(constants.MakerSchema, constants.CatIlkLump)
)
What do you all think?
There was a problem hiding this comment.
👍 I don't think it impacts the readability that much. After FROM my brain just thinks "..what table is this coming FROM?"
There was a problem hiding this comment.
I'm a fan of the change - to me the readability concern is outweighed by the scope of change entailed by updating a table name 👍
| schema := "schema_name" | ||
| table := "table_name" | ||
| fullTableName := shared.GetFullTableName(schema, table) | ||
| Expect(fullTableName).To(Equal("schema_name.table_name")) |
|
|
||
| // storage tables | ||
| const ( | ||
| CdpManagerVat = "cdp_manager_vat" |
There was a problem hiding this comment.
Would be a fan of adding Table to the end of these names so it's clear what purpose they serve when imported in other packages, though not sure whether that could cause collisions with the event tables?
| import ( | ||
| "database/sql" | ||
| "fmt" | ||
| "github.com/makerdao/vdb-mcd-transformers/transformers/shared" |
3f1c398 to
a088fa2
Compare
5cb0c86 to
28c5cc8
Compare
No description provided.