Improve foreign key support #133
Conversation
Codecov Report
@@ Coverage Diff @@
## master #133 +/- ##
==========================================
- Coverage 84.49% 84.45% -0.05%
==========================================
Files 43 44 +1
Lines 3606 3641 +35
==========================================
+ Hits 3047 3075 +28
- Misses 559 566 +7
Continue to review full report at Codecov.
|
Sources/SwiftKuery/Table.swift
Outdated
| /// Function that returns the SQL CREATE TABLE query as a String for this TABLE. | ||
| /// - Returns: A String representation of the table create statement. | ||
| /// - Throws: QueryError.syntaxError if statement build fails. | ||
| public func description(connection: Connection) throws -> String { |
There was a problem hiding this comment.
Was there a reason you moved the describe to the bottom of the class? Just thinking in terms of Jazzy docs since descriptions kind of fits within Initializer?
Sources/SwiftKuery/Table.swift
Outdated
| ```swift | ||
| public class EmployeeTable: Table { | ||
| let tableName = "employeeTable" |
There was a problem hiding this comment.
Were you intending to change these from 4 to 3 spaces? Clogs up the diff a bit.
There was a problem hiding this comment.
The indentation in the two tables in the documentation was a space out, I'll change this and make them both consistent to 4 spaces.
Sources/SwiftKuery/Table.swift
Outdated
| if !foreignKeys.contains(newKey) { | ||
| foreignKeys.append(newKey) | ||
| } else { | ||
| print("Duplicate foreign key detected") |
There was a problem hiding this comment.
I assume this is a print because we still want to continue if we detect duplicate foreign keys. Do we want this to be a log instead of a print through?
There was a problem hiding this comment.
This was debug I had missed during my tidy up, will remove it.
Sources/SwiftKuery/Table.swift
Outdated
| return true | ||
| } | ||
|
|
||
| /// Function that returns the SQL CREATE TABLE query as a String for this TABLE. |
There was a problem hiding this comment.
Not really you but could you add a Usage example to the jazzy docs here since you understand this and are currently editing this section?
| var t2 = Table2() | ||
|
|
||
| // This tests a one to one foreign key | ||
| var createStmt = createTable(t1.foreignKey(t1.a, references: t2.b), connection: connection) |
There was a problem hiding this comment.
The names t1.a are not very helpful for understanding the tests but i realize that you have done this to be consistent with the other tests. Maybe we need another PR to rename all the tests so they can be read more easily
There was a problem hiding this comment.
I think the tests would benefit from a bit of an overhaul in future. I can raise an issue.
kilnerm
force-pushed
the
issue_806
branch
3 times, most recently
from
85f3add to
2b81aba
Compare
Sources/SwiftKuery/Table.swift
Outdated
| // Prints CREATE TABLE ExampleTable (name text) | ||
| ``` | ||
|
|
||
| /// - Returns: A String representation of the table create statement. |
There was a problem hiding this comment.
If these are inside the multiline comment they don't need comment lines here and don't display properly in Jazzy.
Sources/SwiftKuery/ForeignKey.swift
Outdated
| return true | ||
| } | ||
|
|
||
| func describe(queryBuilder: QueryBuilder) -> String { |
There was a problem hiding this comment.
An idea: should we follow the Buidable format and make this the build method?
Sources/SwiftKuery/ForeignKey.swift
Outdated
| } | ||
| keyColumns = keys | ||
| refColumns = refs | ||
| keyNames = keyColumns.map { "\($0._table._name).\($0.name)"} |
There was a problem hiding this comment.
I corrected the spaces at the end of the closure.
If you were referring to the need to pack its unnecessary here as these values will not be used in sql statements.
This PR improves Swift-Kuery's support for foreign keys.
With the changes it is now possible to specify multiple foreign keys on a table.
It should be noted that a user will still need to ensure their tables are built correctly to allow the use of composite keys.