Schema type checker #335
Schema type checker #335
Conversation
|
There's a way to tag people in the commit text:
For me it would be:
|
|
Oh, I think The failed build seems to just be from an unrelated test which happens to fail non-determinstically, using too precise rational comparison. I could fix that too, though I'm not sure if the 1e-15 is supposed to be correct, or just picked arbitrarily. I've noticed a few non-deterministic tests while working on this. |
|
Okay, I fixed the bugs for those corner cases, added some test cases to cover them, and added proper attribution. My only major concern now is that I needed to force push to correct the first commit message, and that may have affected other commits in the middle. I'm newish to git in groups, so let me know if that's an issue, and I can just make another branch without the issue, and issue a new pull request. It does say I want to merge 9 commits in, and that is certainly not the case. Otherwise, unless any of the bikeshedding is cause for concern, I'm happy with the current state of things. I should say I've only tested this superficially on a live project; it was able to detect some bugs in my code in some rarely inserted into tables (due to fields which had been removed in Haskell, but not in the database). But otherwise most of the testing is from the test cases themselves. Primarily they work by taking a schema, using |
|
Thanks for all your hard work here @abigailalice and @KaneTW! There's quite a lot here so it might take me a while to get through this as I'm really busy at work at the moment, but I will try and get to this 🙏 |
|
Take your time, just remember that the middle commits aren't anything related to the actual PR, only the first and last are. |
This makes it very difficult to review. Could you rebase your branch so the changes are on master? Don't worry too much about commit attribution - we squash merge here anyway, so we can sort that separately. |
This closes circuithub#274 and circuithub#186, allowing the generation of CREATE TABLE statements from a TableSchema, as well as checking a TableSchema against a database to determine if the tables are defined correctly in the database to be read from/written to by rel8. It also adds tests for the creation and type checking of tables, to ensure they succeed/fail in appropriate cases. Co-authored-by: David Kraeutmann <[email protected]>
|
Okay @ocharles, I think it should be at an acceptable state to review. Sorry about the difficulty. I should say the issues stated in (1), (2), and (4) in the original PR are still present, assuming anyone feels strongly about them. |
|
@ocharles, is there something preventing this from being accepted, or have you just not had time to get around to it? Not an issue if it's the latter, just wanted to make sure if I misunderstood anything. |
|
@abigailalice purely time constraints I'm afraid! It's still on my radar. I'll be on holiday in a few days so this might need to wait until mid September |
This should close #274 and #186, and potentially #291. The semantics are primarily based on @KaneTW's gist in #274, except with two improvements:
TableSchemawill have identical names, instead of passing silentlyIt also displays information in a slightly different way, trying to show as many errors as it can rather than bailing on the first. It also adds a number of test cases to ensure it behaves correctly for the primitive types, higher kinded types, and a few tests to ensure it fails in cases where it should fail. I'm mostly putting this forward to see what other people think, as I'm not sure if it should have other changes before being accepted. Some of my (admittedly bikesheddy) thoughts on the current state:
Rel8without creating an import cycle, so it's only exported via its own module being public. For some reason the module wouldn't compile unless it importedRel8, getting a type error thatColumnswasn't injective otherwise. I'm assuming the issue is that something not being in scope is effecting type checking, but I've never actually seen an error like that, so really aren't sure. I'm also not really sure ifRel8.Table.Verifyis the best name for the module, I just pickedTablepretty arbitrarily.tests/Rel8/Generic/Rel8able/Test.hsquite a bit to add some things necessary for my own test cases, but I'm not really sure if that's an appropriate place for them. I could move them if that's preferred.Statementtype. I could just add a version usingSessionwhich does this, but for people usinghasql-transactionthey would still need to rewrite that function manually. This isn't a terribly difficult thing for users of the library to do themselves, since anyone using it is already going to need to convert fromStatementtoTransaction, and can just read from the table after type checking, but it just feels like an oversite to not have this. For the moment the issue is explained in the documentation for the function.It should also be said that #59 is kind of exploring similar ideas, and @KaneTW also was exploring some other ideas for interfaces in other changes to the original gist.