-
Notifications
You must be signed in to change notification settings - Fork 292
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Document foreign and primary keys in the quasiquoter better #1308
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! This is awesome work and well-needed.
@@ -134,7 +134,73 @@ This makes a unique index requiring `phone` to be unique across `Person` rows. O | |||
|
|||
## Primary and Foreign keys | |||
|
|||
The [tests for this feature](https://github.com/yesodweb/persistent/blob/master/persistent-test/src/CompositeTest.hs#L53) demonstrates their usage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh my god thank you this is fantastic
docs/Persistent-entity-syntax.md
Outdated
The [tests for composite keys][composite tests] and [tests for foreign keys][foreign tests] have some additional examples. | ||
|
||
[composite tests]: https://github.com/yesodweb/persistent/blob/master/persistent-test/src/CompositeTest.hs#L53 | ||
[foreign tests]: https://github.com/yesodweb/persistent/blob/master/persistent-test/src/ForeignKey.hs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should probably also refer to the Database.Persist.Quasi
documentation here, since a lot of this is covered in that too.
We may want to deprecate the entire file here in favor of that 🤔 But it is nice to have multiple points of contact for folks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we probably do! Or merge anything missing into there. I didn't see that module originally, so the document here should at least be replaced with a link to that.
I'm not sure how to make this more discoverable but it very much needs to be. Maybe pointing to it in the readme if it's not already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you be willing to incorporate your doc additions into the Database.Persist.Quasi
module?
From there I think I can try and restructure the Haddocks to point to that more easily. The module docs for Database.Persist
... could definitely use some polish and pointing towards this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. That was painful. Feel free to directly edit anything you'd like on this branch, I'm too tired to poke at it more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry to hear that it was painful! Thanks for the work you've done, I can take it from here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifically haddock tables have inspired new levels of frustration with the computer. (oh by the way they're a ghc-8.8 feature, we probably don't care because the rendered docs will be available on the web)
Big pain in the butt to make even with relatively advanced vim shenanigans, and it took a while of fiddling around to get them to actually parse. Turns out you need a paragraph between headings and tables apparently. Grrr.
Also I got to deal with the vim highlighting system seemingly not keeping context from off screen so the highlighting on the doc comment kept getting messed up if I moved by pages at a time. Anyway it's done, and I'll get back to using haddock in a more normal amount 😆.
This section got orphaned and I don't know where to put it:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is fantastic, thank you so much!
I've written some documentation based on poking through the tests and my own use of Persistent. There's more work to do in this file, as there are more sections that just tell you to read a test.