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

fix(dojo-lang): use PluginDiagnostic instead of panic for model keys verification #1327

Merged
merged 5 commits into from
Dec 22, 2023

Conversation

glihm
Copy link
Collaborator

@glihm glihm commented Dec 21, 2023

@kariy suggested the use of PluginDiagnostics, which enhance the debug experience:

image

Also, the error handling is now done by model.rs instead of being done inside the Introspect handling.

Happy to have feedback on the wording if it needs to be adjusted. 👍

@glihm glihm changed the title fix: use PluginDiagnostic instead of panic fix(dojo-lang): use PluginDiagnostic instead of panic for model keys verification Dec 21, 2023
@glihm glihm marked this pull request as ready for review December 21, 2023 17:58
@glihm glihm requested review from kariy and tarrencev December 21, 2023 17:59
@codecov-commenter
Copy link

codecov-commenter commented Dec 21, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (e351f0f) 64.27% compared to head (c2dd18c) 64.27%.

❗ Current head c2dd18c differs from pull request most recent head 04e1847. Consider uploading reports for the commit 04e1847 to get more accurate results

Files Patch % Lines
crates/dojo-lang/src/model.rs 71.42% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1327      +/-   ##
==========================================
- Coverage   64.27%   64.27%   -0.01%     
==========================================
  Files         229      229              
  Lines       19711    19708       -3     
==========================================
- Hits        12669    12667       -2     
+ Misses       7042     7041       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@kariy kariy Dec 22, 2023

Choose a reason for hiding this comment

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

i forgot to mention in the initial review, but let's include a test for the new behaviour in here also. Then, we're good to merge.

Copy link
Collaborator Author

@glihm glihm Dec 22, 2023

Choose a reason for hiding this comment

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

Sure thing. Sorry for not putting them at the first round.. Thank you again. 👍

@glihm glihm requested a review from kariy December 22, 2023 21:51
@kariy kariy merged commit 3c55d86 into dojoengine:main Dec 22, 2023
9 checks passed
@glihm glihm deleted the fix/dojo-lang/plugin-diag branch February 17, 2024 20:51
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