Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove the use of load(on:) in relations.md #956

Merged
merged 3 commits into from
Jan 2, 2024
Merged

Remove the use of load(on:) in relations.md #956

merged 3 commits into from
Jan 2, 2024

Conversation

wojexe
Copy link
Contributor

@wojexe wojexe commented Dec 17, 2023

As I've noticed on a discord conversation, the load(on:) method is discouraged to be used and should be replaced with get(on:), which defaults to cache if possible.

@wojexe wojexe requested review from 0xTim and gwynne as code owners December 17, 2023 03:14
@gwynne gwynne added invalid code Code sample not working. no-translation-needed This PR does not require the translations to be updated (e.g. fixing a typo or infrastructure work) labels Dec 17, 2023
@gwynne
Copy link
Member

gwynne commented Dec 17, 2023

Great! Can you also add mention (and possibly an example) of get()'s reload parameter? (The full signature is get(reload:on:), with reload defaulting to false.)

@gwynne gwynne added updates Adding new or updated documentation. and removed no-translation-needed This PR does not require the translations to be updated (e.g. fixing a typo or infrastructure work) labels Dec 17, 2023
@wojexe
Copy link
Contributor Author

wojexe commented Dec 17, 2023

Actually there is a mention of the reload: parameter higher up, in the #get section. I just thought that get(on:) would be sufficient for this use case. But yeah, on the second thought, people might want to have their data reloaded during eager loading, so I'll include it. 😄

0xTim
0xTim approved these changes Jan 2, 2024
View reviewed changes
Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - given the additional text we'll need translations for this so I'll remove the no-translation-needed label

@0xTim 0xTim enabled auto-merge (squash) January 2, 2024 16:54
@0xTim 0xTim merged commit c9dca28 into vapor:main Jan 2, 2024
1 check passed
@penny-for-vapor penny-for-vapor bot mentioned this pull request Jan 2, 2024
Open
9 tasks
alemohamad added a commit to alemohamad/vapor-docs that referenced this pull request Dec 10, 2024
@alemohamad
Translation needed for vapor#956
4016e73
alemohamad added a commit to alemohamad/vapor-docs that referenced this pull request Dec 10, 2024
@alemohamad
Translation needed for vapor#956
4012526
alemohamad added a commit to alemohamad/vapor-docs that referenced this pull request Dec 11, 2024
@alemohamad
Translation needed for vapor#956
611ba54
alemohamad added a commit to alemohamad/vapor-docs that referenced this pull request Dec 13, 2024
@alemohamad
Translation needed for vapor#956
0d784a7
TheHandyOwl pushed a commit to alemohamad/vapor-docs that referenced this pull request Dec 14, 2024
@alemohamad @TheHandyOwl
Translation needed for vapor#956
e565959
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xTim 0xTim 0xTim approved these changes

@gwynne gwynne Awaiting requested review from gwynne gwynne is a code owner

Assignees
No one assigned
Labels
invalid code Code sample not working. updates Adding new or updated documentation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wojexe @gwynne @0xTim