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.

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

Add "create card instance" command #2180

Closed
wants to merge 2 commits into from

Conversation

burieberry
Copy link
Contributor

@burieberry burieberry commented Feb 20, 2025

Open for comments regarding usage of card-resource in a command in this way to create a card. If this is not a good idea, maybe this PR is no longer relevant.

Copy link

github-actions bot commented Feb 20, 2025

Host Test Results

    1 files  ±0      1 suites  ±0   23m 21s ⏱️ - 1m 3s
775 tests ±0  773 ✔️ ±0  2 💤 ±0  0 ±0 
780 runs  ±0  778 ✔️ ±0  2 💤 ±0  0 ±0 

Results for commit 4cc0114. ± Comparison against base commit 1614af1.

♻️ This comment has been updated with latest results.

},
},
};
await this.cardService.createFromSerialized(doc.data, doc);
Copy link
Contributor

Choose a reason for hiding this comment

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

we should really be using the CardResource to make cards, not the CardService directly (it would be great if we could deprecate this so others don't fall into this trap). When you make a card with the CardService directly that means that the card will not be part of the identity map and will not receive live updates, as well as other realy bad side effects.

@burieberry burieberry force-pushed the cs-8032-create-instance-command branch from f577894 to 4cc0114 Compare February 24, 2025 20:50
@burieberry burieberry marked this pull request as ready for review February 24, 2025 21:32
@burieberry burieberry requested a review from a team February 25, 2025 14:48
Copy link
Contributor

@lukemelia lukemelia left a comment

Choose a reason for hiding this comment

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

Result of team review: hold off on this PR, review with @ef4

@burieberry burieberry marked this pull request as draft February 25, 2025 15:24
@burieberry
Copy link
Contributor Author

@burieberry burieberry closed this Mar 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants