-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
docs: update belongsTo docs with decorator syntax #3772
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.
Good start! Let's improve the content 💪
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.
PTAL at my comments below.
I feel it's important to at least mentions the implications on HasMany relation definition. I believe some changes are necessary at HasMany side when the PK is not following our naming convention, we should make this clear to readers (e.g. by referring to the relevant section in HasMany doc page).
@deepakrkris, are you able to update the PR to address comments from @bajtos and @agnes512? Thanks. |
@dhmlau , yes I am able to follow the scope of the review comments. But there are a dozen assumptions on how the simple params keyFrom, keyTo, name are thought to work viz a viz how they actual work (with respect to datasource juggler, etc). because of missing end to end tests and examples for the same. I am carefully going thru each of the comments and testing locally, so I can update with actual content. |
7bfcfc6
to
9c92082
Compare
9c92082
to
089ae42
Compare
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.
Thanks for the PR, @deepakrkris. I have a few comments. Thanks.
89602a6
to
d0ca863
Compare
d0ca863
to
f82c5e8
Compare
@deepakrkris , I'm not sure which PR you want to use, so I've pushed the linting fixes to your other PR. See commit: c28eade. Basically, what I've run is |
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 feel like we are discussing on different things. Please let me know if I misunderstand or miss anything.
Fixes #2639
Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated👉 Check out how to submit a PR 👈